811784 - Account for subscripts when figuring out what object to stick properties on

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

jimb pointed out that we need to put subscripts in our hashtable of script->this object mappings or when Cu.import is called we'll stick properties on the global by accident.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bc502c08da4

Comment 2

[Ed Morley [:emorley]]

Backed out for xpcshell failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aae9b913320f

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Relanded with a fix

https://hg.mozilla.org/integration/mozilla-inbound/rev/a94288026ea5

Comment 4

[Jonathan Griffin (:jgriffin)]

It looks like this broke all B2G emulator tests; re-triggering to verify, since the tests were skipped on this commit.

Comment 5

[Jonathan Griffin (:jgriffin)]

Backed out for turning all b2g emulator tests red:  https://hg.mozilla.org/integration/mozilla-inbound/rev/d770dc9a50e8

For some reason these don't all show up in TBPL, but you can see the carnage at https://secure.pub.build.mozilla.org/buildapi/self-serve/mozilla-inbound/rev/a94288026ea5.

I'll file another bug for the TBPL problem.

Kyle, ping me next week and I can help you reproduce the problem.

Comment 6

[Jonathan Griffin (:jgriffin)]

TBPL problem filed as bug 812709

Comment 7

[Jonathan Griffin (:jgriffin)]

Looked into this a little.  This seems to break the remote debugger in B2G, upon which Marionette depends.  The only error I see in logcat is:

I/Gecko   (  154): 1353120987406	Marionette	ERROR	exception: TypeError, DebuggerServer is undefined

which comes from here:

http://mxr.mozilla.org/mozilla-central/source/testing/marionette/components/marionettecomponent.js#79

I can take a deeper look on Monday.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'd like to fix this but b2g desktop is currently broken.  I filed Bug 813230.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Alternatively you could try opening the Debugger or the addons manager. Both failed for me with this patch.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Additional fix

If we're running a subscript against a global (say a DOM global, or a sandbox), we don't want to execute the subscript as a function, even with the global as the this object, because var/etc won't set properties on the global.

Comment 12

[Alex Keybl [:akeybl]]

This bug has been called out as likely having risk to non-B2G platforms. Given that, marking as P1, and moving into the C2 milestone. We should prioritize this landing to mozilla-beta as soon as possible, to prevent late-breaking regressions to other platforms.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So, actually, this is a bit of a problem.  We really always want to run subscripts at global scope (assuming no target object was specified), but we can't do that if the thing we're importing into is not running at global scope ...

In practice most people specify a target object for loadSubScript, but not everyone does.  And subscripts don't have a well-defined set of symbols to audit like JSMs do.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 683729 [details] [diff] [review]
Additional fix

This is not quite right.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch V2

Ok, this is what we want.  This is the previous patch with one change.  What controls whether we execute the subscript as a script or as a function is whether or not the object that FindTargetObject returns is a global object or not, regardless of whether or not we're actually using the object from FindTargetObject.  This fixes the debugger.

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0f030d7c497f

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Please post changeset links and set the status flags when uplifting.

https://hg.mozilla.org/releases/mozilla-aurora/rev/ac448724f14d

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/releases/mozilla-beta/rev/dde14219c867

Comment 19

[James Lal [:lightsofapollo]]

Seems like this may break httpd.js (which is used by every gaia developer during development much like the debugger). It fits the regression window in that bug.

https://bugzilla.mozilla.org/show_bug.cgi?id=816957

Comment 20

[Ting-Yuan Huang]

Hi Kyle,

The attached patch is different to what was checked in. Is this intended or just a merging mistake?

Specifically,
    options.setSourcePolicy(JS::CompileOptions::LAZY_SOURCE);
was moved into if (!useGlobal):
https://hg.mozilla.org/mozilla-central/rev/0f030d7c497f#l3.73

your patch:
https://bugzilla.mozilla.org/attachment.cgi?id=684111&action=diff#a/js/xpconnect/loader/mozJSSubScriptLoader.cpp_sec4

the previous rev:
https://hg.mozilla.org/mozilla-central/rev/75821787b343

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Well it's been over a year since I landed this code, but I believe I intentionally made that modification.  I believe that, at least as of Gecko 18, trying to use LAZY_SOURCE with a function asserts at http://mxr.mozilla.org/mozilla-b2g18/source/js/src/frontend/BytecodeCompiler.cpp#267.  I do not know whether that restriction is still in effect.

Hope that helps.