Global strict mode function leaking global object as "this".

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark S. Miller]

> function foo(){"use strict"; return this;}
    undefined

    > foo()
    DOMWindow

The call to foo() should have returned undefined.

[Gavin Barraclough]

Trivially, the bug here is that JSGlobalObject doesn't set NeedsThisConversion.  But just setting that may be a performance regression.  (Will make us take unnecessary calls out in non-strict code).

An alternative would be a NeedsStrictThisConversion flag, but it would probably be better to fix jsc so we no longer rely on this conversion to fix up scope objects once we're inside the call, and instead to pass the correct this object into functions in the first place.

[Brendan Eich]

http://code.google.com/p/v8/issues/detail?id=1547 has a testcase in comment 6, correct ES5 output is "oooouuuu", Firefox does "ioiouuuu" for backward compat (but we would like to break that in a coordinated move by browser vendors, with site evangelism as needed).

/be

[Gavin Barraclough]

Created attachment 100706 [details]
Updated test case

[Gavin Barraclough]

Created attachment 100709 [details]
Correction to test case

[Gavin Barraclough]

(In reply to comment #2)
> http://code.google.com/p/v8/issues/detail?id=1547 has a testcase in comment 6, correct ES5 output is "oooouuuu", Firefox does "ioiouuuu" for backward compat (but we would like to break that in a coordinated move by browser vendors, with site evangelism as needed).
> 
> /be

Hey Brendan,

Thanks for the test case.  Just to confirm, the text in the test case seems to be incorrect in stating the expected result to be "iooouuuu", whereas my understanding agrees with your comment that the correct result should be "oooouuuu".

I've expanded the test to cover 'with' behaviour.  I'd expected that the correct this value to be passed in all cases of with statements should be the base object that the property is being accessed off, but looking at the spec (and Safari/FireFox behaviour!) this is incorrect, since the definition for the comma operator causes GetValue to be called on the reference.  It would be great to get confirmation that my understanding here is correct!

WRT to coordinating releases, that would be great.  I'll probably fix this in ToT soon so we can start the bug finding /evangelism process.  I think you guys have plans on a faster release cycle than we do right now - any idea when you'd expect to see this change in a release Firefox?

many thanks,
G.

[Brendan Eich]

(In reply to comment #5)
> (In reply to comment #2)
> > http://code.google.com/p/v8/issues/detail?id=1547 has a testcase in comment 6, correct ES5 output is "oooouuuu", Firefox does "ioiouuuu" for backward compat (but we would like to break that in a coordinated move by browser vendors, with site evangelism as needed).
> > 
> > /be
> 
> Hey Brendan,
> 
> Thanks for the test case.  Just to confirm, the text in the test case seems to be incorrect in stating the expected result to be "iooouuuu", whereas my understanding agrees with your comment that the correct result should be "oooouuuu".

ES5 wants oooouuuu.

Mark, upon hearing the full horror of the web compatibility constraint, wants iooouuuu (he wrote the test, I extended it to test the strict callee g). This is something Mark wants to allow strict callers to protect themselves from leaking the capability to their global object to non-strict callees.

Firefox and IE do ioio.... (IE, even IE10pp, still does not handle strict mode correctly, so gives ioio for .... -- Firefox gives uuuu).

For SpiderMonkey in Firefox, we avoided an extra test for strict caller and wound up with ioiouuuu. We could add the test to avoid the capability leak, but I just today expressed to mark my hope that we can all switch to oooouuuu without breaking the web (maybe with a bit of site evangelism).

> I've expanded the test to cover 'with' behaviour.  I'd expected that the correct this value to be passed in all cases of with statements should be the base object that the property is being accessed off, but looking at the spec (and Safari/FireFox behaviour!) this is incorrect, since the definition for the comma operator causes GetValue to be called on the reference.  It would be great to get confirmation that my understanding here is correct!

You are correct.

> WRT to coordinating releases, that would be great.  I'll probably fix this in ToT soon so we can start the bug finding /evangelism process.  I think you guys have plans on a faster release cycle than we do right now - any idea when you'd expect to see this change in a release Firefox?


Firefox 8, so since we shipped 5 and we ship every quarter, < 3 quarters from now -- early next year.

Question is how many sites will test with our nightly, aurora (think a for alpha) and beta channels, which pipeline-stage every six weeks. But if we get you and V8 to do oooouuuu as well, then *maybe*. The IE share is the worry.

/be

[Gavin Barraclough]

Created attachment 100846 [details]
The patch

Perf tested okay, tested 32bit, 64bit, DFG & interpreter builds.

[Oliver Hunt]

Comment on attachment 100846 [details]
The patch

Check behaviour of getters and setters on the global object, including the cached scenarios.

[Gavin Barraclough]

Created attachment 100875 [details]
Fix for global object getter/setter properties, added test case & ASSERT in JSC::call to guard against global object being passed.

[Oliver Hunt]

Comment on attachment 100875 [details]
Fix for global object getter/setter properties, added test case & ASSERT in JSC::call to guard against global object being passed.

r=me

[Oliver Hunt]

Comment on attachment 100875 [details]
Fix for global object getter/setter properties, added test case & ASSERT in JSC::call to guard against global object being passed.

I think badness can happen in ::execute(Program... ::execute(EvalNode

[Gavin Barraclough]

Created attachment 101020 [details]
Added asserts, changed calls in WebCore/WebKit/WebKit2 to pass correct global object to call.

[WebKit Review Bot]

Attachment 101020 [details] did not pass style-queue:

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

Source/JavaScriptCore/interpreter/Interpreter.h:100:  The parameter name "scopeChain" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Hunt]

Comment on attachment 101020 [details]
Added asserts, changed calls in WebCore/WebKit/WebKit2 to pass correct global object to call.

r=me

[Gavin Barraclough]

I've landed these changes in r91095, Now we just need be vigilant for compatibility issues.

G.

[Simon Fraser (smfr)]

This caused a bunch of new assertions on the bots:
http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r91161%20(32455)/http/tests/inspector/network/network-embed-crash-log.txt

[Gavin Barraclough]

(In reply to comment #16)
> This caused a bunch of new assertions on the bots:
> http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r91161%20(32455)/http/tests/inspector/network/network-embed-crash-log.txt

Cheers Simon, missed those, will investigate.

[Gavin Barraclough]

*** Bug 51097 has been marked as a duplicate of this bug. ***

[Gavin Barraclough]

*** Bug 58338 has been marked as a duplicate of this bug. ***

[Andrew Wason]

The isValidThisObject assert added to Source/JavaScriptCore/runtime/CallData.cpp breaks the Qt JS bridge signal connection functionality in debug builds.
http://doc.qt.nokia.com/latest/qtwebkit-bridge.html#signal-to-function-connections

See bug 66097.

[Gavin Barraclough]

*** Bug 11884 has been marked as a duplicate of this bug. ***