Closed
Bug 885388
Opened 11 years ago
Closed 11 years ago
crash in xpc::WrapperFactory::WaiveXray
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: Honza, Assigned: bholley)
References
Details
(Keywords: crash)
Crash Data
Attachments
(6 files)
2.19 MB,
application/x-xpinstall
|
Details | |
117.86 KB,
application/x-xpinstall
|
Details | |
7.04 KB,
patch
|
jimb
:
review+
|
Details | Diff | Splinter Review |
4.62 KB,
patch
|
jimb
:
review+
|
Details | Diff | Splinter Review |
2.92 KB,
patch
|
jimb
:
review+
|
Details | Diff | Splinter Review |
2.32 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-c9820298-6b98-4683-924a-2288a2130620 . ============================================================= Just fails randomly, could be related (or duplicate?) of bug 885301 Honza
Reporter | ||
Comment 1•11 years ago
|
||
Bobby, do you know what could be going on here? Honza
Assignee | ||
Comment 2•11 years ago
|
||
Something may be going screwy with the waiver map, but it's pretty hard to tell from the crash stack, which is optimized. Are you able to consistently reproduce this?
Reporter | ||
Comment 3•11 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #2) > Are you able to consistently reproduce this? Yes Weird is that I can do it only in some Firefox profiles. It works in new profile till something breaks (I don't know what). If it help I could zip the entire profile together with extensions I have installed (Firebug, Firebug Tracing and Firebug Test Harness) and perhaps you could try it on your machine. Can I send it to your email? Honza
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Jan Honza Odvarko from comment #3) > (In reply to Bobby Holley (:bholley) from comment #2) > > Are you able to consistently reproduce this? > Yes > > Weird is that I can do it only in some Firefox profiles. It works in new > profile till something breaks (I don't know what). > > If it help I could zip the entire profile together with extensions I have > installed (Firebug, Firebug Tracing and Firebug Test Harness) and perhaps > you could try it on your machine. > > Can I send it to your email? Depends. If it only reproduces on windows-opt, I'm not likely to make much progress on it. If you can reproduce it on mac or linux, I might have a better shot.
Reporter | ||
Comment 5•11 years ago
|
||
Here are some STRs that I tried on two clean profiles and it always led to the crash. 1) Create a new Firefox profile 2) Install Firebug and FBTrace extensions (attached) (Firebug first, even if I don't know if it's important) 3) Restart Firefox and load: http://softwareishard.com/har/viewer/?path=examples/inline-scripts-block.har 4) Open Firebug and "Enable All Panels". You can use the popup menu associated with Firebug Start button in Firefox toolbar to enable all panels. 5) Select the Console panel and click Profile button 6) Expand the two "Cuzillion" entries on the page 7) Click the Profile button again to stop profiling 8) Focus Firefox address bar (click on it) and press the Enter key -> CRASH If it doesn't crash the first time restart Firefox and try again (from step #3). It took me 2-3 restarts to crash Firefox. --- Can you reproduce the problem now? Honza
Assignee | ||
Comment 6•11 years ago
|
||
(In reply to Jan Honza Odvarko from comment #5) > Can you reproduce the problem now? On Mac debug, I cannot. A few oddities with the STR: * I didn't see any attachment in the bug, so I installed Firebug 1.11.4 from AMO, and FBTrace 1.11.2 from https://getfirebug.com/releases/fbtrace/1.11/ * In order make "enable all panels" have any effect, I need to first enable firebug (by clicking on the bug). * The 'profile' button is unclickable until I reload the page at least once after Firebug has been activated.
Reporter | ||
Comment 7•11 years ago
|
||
Reporter | ||
Comment 8•11 years ago
|
||
Reporter | ||
Comment 9•11 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #6) > (In reply to Jan Honza Odvarko from comment #5) > > Can you reproduce the problem now? > > On Mac debug, I cannot. > > A few oddities with the STR: > > * I didn't see any attachment in the bug, so I installed Firebug 1.11.4 from > AMO, and FBTrace 1.11.2 from https://getfirebug.com/releases/fbtrace/1.11/ Sorry, now I attached the XPIs. > * In order make "enable all panels" have any effect, I need to first enable > firebug (by clicking on the bug). Yep, that's correct > * The 'profile' button is unclickable until I reload the page at least once > after Firebug has been activated. Please try again with the XPIs I have attached. Thanks! (this bug is annoying and is blocking us to fully adopt JSD2 in Firebug) Honza
Comment 10•11 years ago
|
||
Jim, bholley and I discussed this on IRC; here are some preliminary findings: - These compilation-only compartments are fairly normal actually. The globals aren't XPConnect objects. Evaluating arbitrary code in such a compartment, as debuggers are wont to do, would probably be fine... modulo bugs, and it's plausible that there could be a bug or two. bholley does not want to spend time tracking down the bug or two, given that it's such a perverse thing to attempt in the first place. - The thing we *know* crashes is trying to create certain kinds of cross-compartment edges (in this case, from chrome into the compilation compartment). To XPConnect the existence of these compartments is purely an implementation detail, and nobody should ever request a cross-compartment wrapper pointing into or out of such a compartment. - The edge from a Debugger.Object to its referent is not per se dangerous. However certain operations on a Debugger.Object carry the danger of trying to create cross-compartment wrappers that bholley really does not want to support in xpconnect/wrappers. In particular: Debugger.Object.p.apply and others can create edges from any compartment to any other compartment. Debugger.Object.p.unsafeDereference creates one such edge and from that point on, script can unwittingly ask for all sorts of other such edges. I don't know if it is necessary to observe these globals in order to get notification on all new scripts/sources. I told bholley it is *definitely* OK to add a CompartmentOption stating that a particular new compartment is compile-only, with detailed semantic consequences to be determined. bholley would like the consequences to be "nothing in this compartment is ever reported to the debugger" but I advised that the debugger might need limited access. And I think that's OK; there are choke points in vm/Debugger.cpp where we could prevent certain uses of objects in this sort of compartment. (I'm thinking of DebuggerObject_checkThis and Debugger::unwrapDebuggeeValue.)
Assignee | ||
Comment 11•11 years ago
|
||
(In case comment 10 didn't make it clear, I've determined the cause of the bug and am determining the appropriate fix)
Assignee: nobody → bobbyholley+bmo
Reporter | ||
Comment 12•11 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #11) > (In case comment 10 didn't make it clear, I've determined the cause of the > bug and am determining the appropriate fix) Excellent, thanks! Honza
Comment 13•11 years ago
|
||
So is the idea here that we do the compilation in one compartment, and then clone the scripts into other compartments where they actually execute? It doesn't look to me as if we watch script cloning carefully enough that we'd always get the notifications we need. For example, JS_ExecuteScript calls CloneScript, but never calls onNewScript. This seems like an existing bug; I gather our tests don't exercise cloning very well.
Assignee | ||
Comment 14•11 years ago
|
||
(In reply to Jim Blandy :jimb from comment #13) > So is the idea here that we do the compilation in one compartment, and then > clone the scripts into other compartments where they actually execute? Yes. So it has been since long before the sun burned hot in space. > It doesn't look to me as if we watch script cloning carefully enough that > we'd always get the notifications we need. For example, JS_ExecuteScript > calls CloneScript, but never calls onNewScript. This seems like an existing > bug; I gather our tests don't exercise cloning very well. Ok. So can we safely just disable debugger events for those scopes? If we want to fire onNewScript when we clone the script into a real usable compartment, that's fine (and can be handled separately).
Flags: needinfo?(jimb)
Comment 15•11 years ago
|
||
So, as it's been explained to me, the main issue here that Debugger facilities like the onNewGlobalObject hook and addAllGlobalsAsDebuggees are finding compartments that the browser (Jason called it XPConnect above, but bholley says it's the browser) considers its personal implementation details, and not appropriate for use by other things. Debugger makes it possible to interfere with things that used to be invisible to the rest of the system. Since these things aren't useful to debug anyway, and the ability to do so introduces new classes of failures, the request is to make these globals invisible to the debugger. onNewGlobalObject and addAllGlobalsAsDebuggees are really pretty weird fruit to begin with, in that they trawl the entire system for specific objects in a way ordinary code is unable to (and this is why they're marked as 'privileged code only' in the docs). They only exist to support chrome debugging, so it should be fine to mark some globals as "invisible to the debugger". That would be an easy change. This flag would probably be appropriate for the compartment in which self-hosted code is compiled, as well.
Flags: needinfo?(jimb)
Assignee | ||
Comment 16•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=462092577316
Assignee | ||
Comment 17•11 years ago
|
||
One small bug revealed by bc on the try push, which I've now fixed. Uploading patches and flagging for review.
Assignee | ||
Comment 18•11 years ago
|
||
Attachment #781095 -
Flags: review?(jimb)
Assignee | ||
Comment 19•11 years ago
|
||
Originally, this thing took a string ('same-compartment' or 'new-compartment'), which became unused with CPG, though is still passed by a number of tests. Then it looks like billm made it take an object in bug 852228, for sameZoneAs, but bugzilla and grep indicate that it never got used. Let's switch it to something extensible, though we should also make sure to let Jesse, decoder, gwk, and any other fuzzing folk know about it.
Attachment #781096 -
Flags: review?(jimb)
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #781097 -
Flags: review?(jimb)
Assignee | ||
Comment 21•11 years ago
|
||
These are the two main ones I can think of where we want to do this. Can anyone think of any others?
Attachment #781099 -
Flags: review?(mrbkap)
Comment 22•11 years ago
|
||
Comment on attachment 781095 [details] [diff] [review] Part 1 - Introduce a mechanism to make certain globals invisible to the debugger. v1 Review of attachment 781095 [details] [diff] [review]: ----------------------------------------------------------------- This looks great, with the comment addressed. ::: js/src/vm/Debugger.h @@ +684,5 @@ > void > Debugger::onNewGlobalObject(JSContext *cx, Handle<GlobalObject *> global) > { > + if (!JS_CLIST_IS_EMPTY(&cx->runtime()->onNewGlobalObjectWatchers) && > + !global->compartment()->options().invisibleToDebugger) I think the test of the invisibleToDebugger flag belongs in Debugger::slowPathOnNewGlobalObject. The fast path inlined functions are meant to minimize impact when the debugger isn't present at all, and should not concern themselves with rare cases.
Attachment #781095 -
Flags: review?(jimb) → review+
Comment 23•11 years ago
|
||
Comment on attachment 781096 [details] [diff] [review] Part 2 - Alter the newGlobal API in the shell to take an extensible options object. v1 Review of attachment 781096 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/shell/js.cpp @@ +3549,2 @@ > > + if (argc == 1 && JS_ARGV(cx, vp)[0].isObject()) { I think we prefer to use CallArgs instead of JS_ARGV in the shell.
Attachment #781096 -
Flags: review?(jimb) → review+
Comment 24•11 years ago
|
||
Comment on attachment 781097 [details] [diff] [review] Part 3 - Hook up invisibleToDebugger to js shell and add test coverage. v1 Review of attachment 781097 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Please add test coverage for Debugger.prototype.addAllGlobalsAsDebuggees, too.
Attachment #781097 -
Flags: review?(jimb) → review+
Updated•11 years ago
|
Attachment #781099 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 25•11 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ca063a0366e0 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e43bd9209569 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5c67d8a21748 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/466127b4c3b2
Comment 26•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ca063a0366e0 https://hg.mozilla.org/mozilla-central/rev/e43bd9209569 https://hg.mozilla.org/mozilla-central/rev/5c67d8a21748 https://hg.mozilla.org/mozilla-central/rev/466127b4c3b2
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment 27•11 years ago
|
||
I am seeing this crash again: https://crash-stats.mozilla.com/report/index/bdf4aa2c-a3b9-4281-8d8e-770e52130918 Should I create a new issue? Or should this one be reopened? Sebastian
Comment 28•11 years ago
|
||
Given stack frames 3 and 4, this might be related to off-mainthread script parsing: 3 PR_AttachThread nsprpub/pr/src/threads/prcthr.c 4 xul.dll mozilla::dom::workers::scriptloader::ReportLoadError(JSContext *,nsString const &,tag_nsresult,bool) @bhackett, do you agree?
Flags: needinfo?(bhackett1024)
Comment 29•11 years ago
|
||
(In reply to Sebastian Zartner from comment #27) > I am seeing this crash again: > https://crash-stats.mozilla.com/report/index/bdf4aa2c-a3b9-4281-8d8e- > 770e52130918 > > Should I create a new issue? Or should this one be reopened? I would say that as something has been fixed here and it has been a while, it's probably best to go for a new bug for that issue.
Comment 30•11 years ago
|
||
> I would say that as something has been fixed here and it has been a while, it's probably best to go > for a new bug for that issue. Bug 917780. Sebastian
You need to log in
before you can comment on or make changes to this bug.
Description
•