Closed Bug 885388 Opened 6 years ago Closed 6 years ago

crash in xpc::WrapperFactory::WaiveXray

Categories

(Core :: XPConnect, defect, critical)

x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: Honza, Assigned: bholley)

References

Details

(Keywords: crash)

Crash Data

Attachments

(6 files)

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
Bobby, do you know what could be going on here?
Honza
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?
(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
(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.
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
(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.
(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
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.)
(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
(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
Depends on: 890576
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.
(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)
Depends on: 897322
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)
One small bug revealed by bc on the try push, which I've now fixed. Uploading patches and flagging for review.
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)
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 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 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 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+
Attachment #781099 - Flags: review?(mrbkap) → review+
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
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)
(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.
> 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
Moving needinfo to the new bug
Flags: needinfo?(bhackett1024)
You need to log in before you can comment on or make changes to this bug.