Bug 33057 - REGRESSION(r49365): typeof(xhr.responseText) != "string" in Windows
Summary: REGRESSION(r49365): typeof(xhr.responseText) != "string" in Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P1 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-30 09:51 PST by Marshall Culpepper
Modified: 2010-01-07 16:17 PST (History)
8 users (show)

See Also:


Attachments
proposed fix (24.71 KB, patch)
2010-01-07 15:42 PST, Alexey Proskuryakov
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marshall Culpepper 2009-12-30 09:51:18 PST
Hey guys.. this is a really odd one we noticed in the latest builds of the Win32/Cairo port, Brent Fulgham also verified this is happening in the official Windows port.

Basically in an XHR response, the responseText is correctly filled with data, but the "typeof" the responseText won't correctly compare against the string "string", unless the typeof or the responseText itself is forced into a String context. To clarify, here are a few examples:

"type="+typeof(xhr.responseText) // -> type=string

typeof(xhr.responseText) == "string" // false, should be true

String(typeof(xhr.responseText)) == "string" // true

typeof(String(xhr.responseText)) == "string" // true

var t = typeof(xhr.responseText);
t == "string" // true .. weird right?

tyepof(xhr.responseText).substr(0,5) == "string" // true

It should be noted that performing any standard String operations on the responseText or the typeof seem to work fine.

If you load this test case in WinLauncher, you should see all red text with typeof(responseText) == "string" ? false :
http://arcaner.com/test/test.html
Comment 1 Alexey Proskuryakov 2009-12-30 17:58:34 PST
Just to clarify, did this work before? Can you check when exactly this broke (which is the latest nightly build where this works)?

Sounds like this could be related to recent JavaScript string refactoring work. I can't reproduce this with nightly r52591 on Mac OS X, and didn't try on Windows though.
Comment 2 Martin Robinson 2009-12-31 19:07:22 PST
A little more information: we are only seeing this on the Windows builds. Neither the Mac build or the Linux GTK+ port show this behavior. I'm not sure how long this bug has been around, but I remember seeing it for at least a month. We can try to narrow it down a little.
Comment 3 Alexey Proskuryakov 2010-01-01 19:31:40 PST
I cannot reproduce this on Windows with nightly r52686. Please attach a complete reduced test case.
Comment 4 Martin Robinson 2010-01-05 10:48:59 PST
It could be that this is confined to the Win32 Cairo port. I'll double-check with the original reporter and update this ticket. Sorry for the confusion.
Comment 5 Alexey Proskuryakov 2010-01-06 16:07:40 PST
I can reproduce this now. I don't know what went wrong with my testing before. The only difference I'm aware of is that I was testing with XP, and am testing with Vista now - but that shouldn't affect behavior.

I'm working on a fix.
Comment 6 Alexey Proskuryakov 2010-01-06 19:48:38 PST
The problem is due to the fact that jsString() and related functions are now inline, so JSString objects constructed from WebCore get a different virtual function table pointer. This breaks isJSString() function. I have added appropriate assertions to constructors, and saw them fire:

ASSERT(vptr() == globalData->jsStringVPtr);

The common way of telling the compiler to use the same vtable in DLL and EXE is to use __declspec(dllimport/dllexport). And this fixes the assertion indeed (that's equivalent to what we have on Mac OS X). But it doesn't fix the actual bug!

Turns out that MSVC changes the vptr AFTER constructor exits, trying to fix bugs in certain kinds of code for programmers: <http://groups.google.com/group/microsoft.public.vc.language/msg/55cdcefeaf770212>. I couldn't find a way to disable this behavior. Some possible ways to fix this are:
- don't construct inline (or only do that when building JSC);
- fix vptr in jsString() after constructing the object;
- check for type in some other way (maybe typeid().name, like dynamic_cast does? I'm not sure if that works without RTTI though, or if MSVC breaks typename identity across DLL boundaries, too).

Suggestions are welcome.
Comment 7 Alexey Proskuryakov 2010-01-07 15:42:47 PST
Created attachment 46093 [details]
proposed fix
Comment 8 WebKit Review Bot 2010-01-07 15:47:40 PST
Attachment 46093 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/cf/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/runtime/JSString.h:359:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 9
Comment 9 Geoffrey Garen 2010-01-07 15:58:20 PST
Comment on attachment 46093 [details]
proposed fix

r=me
Comment 10 Alexey Proskuryakov 2010-01-07 16:17:32 PST
Committed <http://trac.webkit.org/changeset/52956>.

This bug also affected PaceKeeper benchmark for Windows Safari, which now works for me.

For the record, the issue I had with my initial testing was that I was using Safari 4.0.3 - after updating to 4.0.4 on that machine, I could reproduce.