Bug 58747 - REGRESSION (r84094): Many tests failing assertions in WebCore::localizedString on Windows
Summary: REGRESSION (r84094): Many tests failing assertions in WebCore::localizedStrin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Windows 7
: P2 Normal
Assignee: Nobody
URL:
Keywords: LayoutTestFailure, PlatformOnly, Regression
Depends on:
Blocks:
 
Reported: 2011-04-17 13:05 PDT by Jessie Berlin
Modified: 2011-04-18 15:32 PDT (History)
5 users (show)

See Also:


Attachments
Proposed Change to fix the update-webkit-localizable-strings script (14.11 KB, patch)
2011-04-18 10:14 PDT, Timothy Hatcher
mitz: review+
timothy: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jessie Berlin 2011-04-17 13:05:37 PDT
http://trac.webkit.org/changeset/84094

http://build.webkit.org/builders/Windows%20XP%20Debug%20%28Tests%29/builds/27726
http://build.webkit.org/results/Windows%20XP%20Debug%20(Tests)/r84095%20(27727)/results.html

Looking at the crash logs, I think this is a problem for not just Windows XP, but all Windows ports. Since it is trying to write to bbadbeef, it is probably the ASSERT(isMainThread()) that is getting hit in debug builds (where as all the other Windows test bots are Release).

I am not sure yet where it is being called from a non-main thread. We should consider rolling back this change if it isn’t fixed soon.
Comment 1 Adam Roben (:aroben) 2011-04-17 14:15:30 PDT
Here's the assertion:

http://trac.webkit.org/browser/trunk/Source/WebCore/platform/win/LocalizedStringsWin.cpp?rev=84087#L82

    ASSERT_WITH_MESSAGE(result.get() != notFound, "could not find localizable string %s in bundle", key);
Comment 2 Jessie Berlin 2011-04-18 06:55:39 PDT
(In reply to comment #1)
> Here's the assertion:
> 
> http://trac.webkit.org/browser/trunk/Source/WebCore/platform/win/LocalizedStringsWin.cpp?rev=84087#L82
> 
>     ASSERT_WITH_MESSAGE(result.get() != notFound, "could not find localizable string %s in bundle", key);

The assertion failure seems to be happening under places like WebCore::imageTitle. Wouldn't it be expected to not find all image titles localized in the webkit bundle?

Maybe that assertion needs to removed and replaced with:

if (result.get() != notFound)
    return result.get();

and otherwise let it fall through to the non-cf version? (which just returns the string itself?)

Or maybe we shouldn't be trying to localize image titles at all?
Comment 3 Adam Roben (:aroben) 2011-04-18 07:18:32 PDT
(In reply to comment #2)
> Wouldn't it be expected to not find all image titles localized in the webkit bundle?

Yes. But we aren't trying to find all image titles. We're trying to find a format string: "%@ $@x$@ pixels".
Comment 4 Adam Roben (:aroben) 2011-04-18 07:18:45 PDT
I had some typos in that format string, but you get the idea.
Comment 5 Timothy Hatcher 2011-04-18 10:14:14 PDT
Created attachment 90047 [details]
Proposed Change to fix the update-webkit-localizable-strings script
Comment 6 WebKit Review Bot 2011-04-18 10:16:08 PDT
Attachment 90047 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Timothy Hatcher 2011-04-18 10:35:54 PDT
Comment on attachment 90047 [details]
Proposed Change to fix the update-webkit-localizable-strings script

Landed in r84158. Build fix in r84159.
Comment 8 Jessie Berlin 2011-04-18 15:32:27 PDT
Fixed in http://trac.webkit.org/changeset/84192 by copying over the proper Localizable.strings file.