Closed
Bug 811784
Opened 12 years ago
Closed 12 years ago
Account for subscripts when figuring out what object to stick properties on
Categories
(Core :: XPConnect, defect, P1)
Core
XPConnect
Tracking
()
People
(Reporter: khuey, Assigned: khuey)
References
Details
Attachments
(1 file, 2 obsolete files)
9.42 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | 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)
Assignee | ||
Updated•12 years ago
|
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Updated•12 years ago
|
Attachment #681549 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 1•12 years ago
|
||
Comment 2•12 years ago
|
||
Backed out for xpcshell failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aae9b913320f
Assignee | ||
Comment 3•12 years ago
|
||
Relanded with a fix
https://hg.mozilla.org/integration/mozilla-inbound/rev/a94288026ea5
Comment 4•12 years ago
|
||
It looks like this broke all B2G emulator tests; re-triggering to verify, since the tests were skipped on this commit.
Comment 5•12 years ago
|
||
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•12 years ago
|
||
TBPL problem filed as bug 812709
Comment 7•12 years ago
|
||
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.
Assignee | ||
Comment 9•12 years ago
|
||
I'd like to fix this but b2g desktop is currently broken. I filed Bug 813230.
Comment 10•12 years ago
|
||
Alternatively you could try opening the Debugger or the addons manager. Both failed for me with this patch.
Assignee | ||
Comment 11•12 years ago
|
||
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)
Comment 12•12 years ago
|
||
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)
Assignee | ||
Comment 13•12 years ago
|
||
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.
Assignee | ||
Comment 14•12 years ago
|
||
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)
Assignee | ||
Comment 15•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #684111 -
Flags: review?(mrbkap) → review+
Comment 16•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 17•12 years ago
|
||
Please post changeset links and set the status flags when uplifting.
https://hg.mozilla.org/releases/mozilla-aurora/rev/ac448724f14d
status-firefox19:
--- → fixed
status-firefox20:
--- → fixed
Assignee | ||
Comment 18•12 years ago
|
||
status-firefox18:
--- → fixed
Comment 19•12 years ago
|
||
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 → ---
Assignee | ||
Updated•12 years ago
|
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Whiteboard: [status-b2g18:fixed]
Updated•12 years ago
|
status-b2g18:
--- → fixed
Whiteboard: [status-b2g18:fixed]
Comment 20•11 years ago
|
||
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)
Assignee | ||
Comment 21•11 years ago
|
||
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.
Description
•