Closed Bug 811784 Opened 8 years ago Closed 8 years ago
Account for subscripts when figuring out what object to stick properties on
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: --- → ?
blocking-basecamp: ? → +
Attachment #681549 - Flags: review?(mrbkap) → review+
Backed out for xpcshell failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/aae9b913320f
Relanded with a fix https://hg.mozilla.org/integration/mozilla-inbound/rev/a94288026ea5
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.
Duplicate of this bug: 812615
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.
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.
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.
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 #684111 - Flags: review?(mrbkap) → review+
Status: NEW → RESOLVED
Closed: 8 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: 8 years ago → 8 years ago
Resolution: --- → 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
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.
You need to log in before you can comment on or make changes to this bug.