Closed Bug 811784 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: XPConnect, defect, P1)

defect

Tracking

()

RESOLVED FIXED
B2G C2 (20nov-10dec)
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: khuey, Assigned: khuey)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #681549 - Flags: review?(mrbkap)
blocking-basecamp: --- → ?
Attachment #681549 - Flags: review?(mrbkap) → review+
It looks like this broke all B2G emulator tests; re-triggering to verify, since the tests were skipped on this commit.
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.
TBPL problem filed as bug 812709
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.
I'd like to fix this but b2g desktop is currently broken.  I filed Bug 813230.
Alternatively you could try opening the Debugger or the addons manager. Both failed for me with this patch.
Attached patch Additional fix (obsolete) — Splinter Review
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.
Attachment #683729 - Flags: review?(mrbkap)
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.
Priority: -- → P1
Target Milestone: --- → B2G C2 (20nov-10dec)
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 on attachment 683729 [details] [diff] [review]
Additional fix

This is not quite right.
Attachment #683729 - Attachment is obsolete: true
Attachment #683729 - Flags: review?(mrbkap)
Attached patch Patch V2Splinter Review
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.
Attachment #681549 - Attachment is obsolete: true
Attachment #684111 - Flags: review?(mrbkap)
Attachment #684111 - Flags: review?(mrbkap) → review+
https://hg.mozilla.org/mozilla-central/rev/0f030d7c497f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Please post changeset links and set the status flags when uplifting.

https://hg.mozilla.org/releases/mozilla-aurora/rev/ac448724f14d
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [status-b2g18:fixed]
Whiteboard: [status-b2g18:fixed]
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
Flags: needinfo?(khuey)
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.
Flags: needinfo?(khuey)
You need to log in before you can comment on or make changes to this bug.