This bug was filed from the Socorro interface and is report bp-d2b6ee93-7d37-4078-a801-a34c02130322 . ============================================================= STR: I tried to open the links in bug 853772 comment 3 mozilla::dom::InstanceClassHasProtoAtDepth dom/bindings/BindingUtils.cpp:619 TestShouldDOMCall js/src/ion/IonBuilder.cpp:4215 mozilla::ErrorResult::ReportJSException dom/bindings/BindingUtils.cpp:154 PR_GetCurrentThread nsprpub/pr/src/pthreads/ptthread.c:621 js::ion::IonBuilder::makeCallHelper js/src/ion/IonBuilder.cpp:4341 js::TempAllocPolicy::malloc_ obj-firefox/dist/include/js/Utility.h:151 js::ion::IonBuilder::makeCallBarrier js/src/ion/IonBuilder.cpp:4380 js::ion::IonBuilder::jsop_call js/src/ion/IonBuilder.cpp:4160 js::ion::IonBuilder::getPropTryConstant js/src/ion/IonBuilder.cpp:6563 js::ion::MUnbox::New js/src/ion/MIR.h:1872 PR_GetCurrentThread nsprpub/pr/src/pthreads/ptthread.c:621 js::ion::MPassArg::New js/src/ion/MIR.h:2104 js::ion::IonBuilder::inspectOpcode js/src/ion/IonBuilder.cpp:932 js::ion::MBasicBlock::NewPopN js/src/ion/MIRGraph.cpp:127 PR_GetCurrentThread nsprpub/pr/src/pthreads/ptthread.c:621 PR_GetThreadPrivate nsprpub/pr/src/threads/prtpd.c:200 js::ion::IonBuilder::processCondSwitchBody js/src/ion/IonBuilder.cpp:2503 js::ion::IonBuilder::processCondSwitchCase js/src/ion/IonBuilder.cpp:2474 js::ion::IonBuilder::processCondSwitchBody js/src/ion/IonBuilder.cpp:2503 js::ion::IonBuilder::snoopControlFlow js/src/ion/CompileInfo-inl.h:56 js::ion::IonBuilder::traverseBytecode js/src/ion/IonBuilder.cpp:684 js::ion::IonBuilder::build js/src/ion/IonBuilder.cpp:346 js::VectorImpl<js::types::CompilerOutput, 0ul, js::TempAllocPolicy, false>::grow obj-firefox/dist/include/js/Utility.h:168 js::ion::SequentialCompileContext::compile js/src/ion/Ion.cpp:1293 js::ion::IonBuilder::IonBuilder js/src/ion/IonBuilder.cpp:48 js::ion::Compile<js::ion::SequentialCompileContext> js/src/ion/Ion.cpp:1248 icon-theme.cache@0x135e000 js::CallObject::createForFunction js/src/gc/Barrier-inl.h:74 js::CallObject::createForFunction js/src/gc/Barrier-inl.h:74 js::ion::CanEnterAtBranch js/src/ion/Ion.cpp:1514 js::Interpret js/src/jsinterp.cpp:1591 [...]

A null-deref on the domClass? That's ... very odd. Mats, can you get this in a debugger? I'd very much like to interrogate the objects involved, but I can't reproduce the crash. :(

I should have noted that the crash was with a non-default profile, sorry. I've reduced the prefs.js that causes the crash to this: user_pref("javascript.options.methodjit.content", false); Boris, can you reproduce it with that?

Yes, indeed. In fact, in a debug build I get a fatal assert on the toPrivate() bit of this line: js::GetReservedSlot(protoObject, DOM_PROTO_INSTANCE_CLASS_SLOT).toPrivate()); in mozilla::dom::InstanceClassHasProtoAtDepth. The passed-in protoObject is "Worker". Not "WorkerPrototype", but "Worker". In particular, its getClass is Worker::sClass. Its class flags are 0x330. I assume this is part of the worker "use instance classes for prototypes but still use DOMJSClass for instances" insanity, but I can't find where workers are setting up their proto objects right now. That spot needs to be fixed to not violate invariants. The current setup is ... a problem.

It's possible that workers create the proto via the mess that is js::InitClassWithReserved...

This is all totally busted. So. 1) Workers use the same JSClass for instances and prototypes. 2) The trace hook in this class tries to UnwrapDOMObject to WorkerPrivate and it it returns non-null (it'll return null for the proto!) traces it. 3) The DOM object is stored in slot 0. 4) The instance class is stored on normal WebIDL prototypes in slot 0. So the only possible solution I can see here is to stop the insanity and stop using the same JSClass for both instances and protos for workers. Ben, do we do that for anything other than Worker, ChromeWorker, and DedicatedWorkerGlobalScope? On the other hand, if we do that, how do we make the objects have the right proto? Right now it seems like it finds them via the insane jsclass-to-proto hashtable thing, right? I guess another possible solution is to completely disable the ion DOM optimizations because of this worker insanity. :(

And to be clear, this "reuse the same class" thing was supposed to be temporary when it was written over a year ago. Since then, it's caused at least two security bugs that I know of. We really need someone to actually fix it. :(

This almost certainly affects every single branch we have that has Ion...

We have no info around when this regressed or the level of criticality here and are about to build our final FF20 beta & rc candidate so minusing for tracking on FF20 and we can make a call on tracking for other versions once we get more info.

> We have no info around when this regressed This regressed with the ionmonkey landing in 18. > or the level of criticality here That's a harder call. If the private on these is always JS::UndefinedValue, then it's possible that this is always a safe crash. I just can't tell obviously from code inspection.... Certainly this is a trivial "crash the browser" kind of thing, but that's not that critical in some ways.

I talked this over with mrbkap yesterday and I think splitting the worker classes into a instance and prototype classes is fine if it makes life easier. There's nothing really "insane" going on here, though... js::InitClass with a null private and a constructor function is just the way the JS engine does classes. It was the way we approximated DOM constructors back when this was written years ago. As we continue to aggressively optimize JS/DOM interaction we're bound to run into cases where this old approximation causes problems.

> There's nothing really "insane" going on here, though... There is from the point of view of WebIDL: prototypes and instance objects are totally different beasts in WebIDL, with different behavior, different [[Class]], etc... The other "insane" part is manually doing things that sort of look like WebIDL objects but violate invariants WebIDL objects depend on, of course. :( In any case, if we plan to split this up, who's doing the work? You, me, or Kyle seem like the obvious options...

(In reply to Boris Zbarsky (:bz) from comment #11) Right, I'm in total agreement. I guess my point was just that this is old code written before WebIDL was a real thing and we're trying to retrofit it, basically. Incompatibilities that require code changes are inevitable. > In any case, if we plan to split this up, who's doing the work? I'm really hoping Kyle can tackle this. I'm utterly swamped for the next few weeks at least.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=bbfddd3e5b52

I'm just going to mark this as sec-high, because of unknown badness. Feel free to adjust as needed. Marking esr17 and b2g18 as unaffected, as they don't have Ionmonkey.

Comment on attachment 730123 [details] [diff] [review] part 1. Give workers that are pretending to be on WebIDL bindings separate JSClasses for instance objects and prototype objects. Review of attachment 730123 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/Worker.cpp @@ +481,5 @@ > + // js::InitClassWithReserved this JSClass and then call > + // JS_NewObject with our sClass and have it find the right > + // prototype. > + "ChromeWorker", > + JSCLASS_IS_DOMIFACEANDPROTOJSCLASS | JSCLASS_HAS_RESERVED_SLOTS(2), This only needs one reserved slot, right? ::: dom/workers/WorkerScope.cpp @@ +895,5 @@ > + // so that we can JS_InitClass this JSClass and then > + // call JS_NewObject with our sClass and have it find the right > + // prototype. > + "DedicatedWorkerGlobalScope", > + JSCLASS_IS_DOMIFACEANDPROTOJSCLASS | JSCLASS_HAS_RESERVED_SLOTS(2), Same here.

Comment on attachment 730124 [details] [diff] [review] part 2. Remove a null check in bindings code that is no longer needed because workers no longer use a DOMJSClass for prototype objects. Review of attachment 730124 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine to me, but I bet you want peterv to look over this.

> This only needs one reserved slot, right? No. The second one is used by Xrays.

Comment on attachment 730123 [details] [diff] [review] part 1. Give workers that are pretending to be on WebIDL bindings separate JSClasses for instance objects and prototype objects. Review of attachment 730123 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/Worker.cpp @@ +316,5 @@ > + { > + // XXXbz we use "Worker" here to match sClass so that we can > + // js::InitClassWithReserved this JSClass and then call > + // JS_NewObject with our sClass and have it find the right > + // prototype. Bah :-(. These should just be in the DOM proto array :-(. @@ +336,5 @@ > + JSCLASS_NO_INTERNAL_MEMBERS > + }, > + eInterfacePrototype, > + &sWorkerNativePropertyHooks, > + "[object Worker]", "[object WorkerPrototype]" I think. @@ +499,5 @@ > + JSCLASS_NO_INTERNAL_MEMBERS > + }, > + eInterfacePrototype, > + &sWorkerNativePropertyHooks, > + "[object ChromeWorker]", "[object ChromeWorkerPrototype]" ::: dom/workers/WorkerScope.cpp @@ +913,5 @@ > + JSCLASS_NO_INTERNAL_MEMBERS > + }, > + eInterfacePrototype, > + &sWorkerNativePropertyHooks, > + "[object DedicatedWorkerGlobalScope]", "[object DedicatedWorkerGlobalScopePrototype]"

> Bah :-(. These should just be in the DOM proto array :-(. I agree, but that's a larger change that I'm not comfortable backporting to branches. :( > "[object WorkerPrototype]" I think. Again, I was trying to preserve existing behavior as much as possible. Are you OK with me not making that change?

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e18a5b5257d https://hg.mozilla.org/integration/mozilla-inbound/rev/db2babefda12

Comment on attachment 730123 [details] [diff] [review] part 1. Give workers that are pretending to be on WebIDL bindings separate JSClasses for instance objects and prototype objects. [Approval Request Comment] Bug caused by (feature/regressing bug #): Ionmonkey landing User impact if declined: Possible crashes in ion code that involves workers, may be exploitable. Testing completed (on m-c, etc.): Passes tests Risk to taking this patch (and alternatives if risky): Risk is low to medium. The other option is disabling the ion optimizations for WebIDL bindings, which I suspect is riskier, not to mention a performance hit. String or IDL/UUID changes made by this patch: None.

(In reply to Boris Zbarsky (:bz) from comment #23) > Comment on attachment 730123 [details] [diff] [review] > part 1. Give workers that are pretending to be on WebIDL bindings separate > JSClasses for instance objects and prototype objects. > > [Approval Request Comment] > Bug caused by (feature/regressing bug #): Ionmonkey landing > User impact if declined: Possible crashes in ion code that involves workers, > may > be exploitable. > Testing completed (on m-c, etc.): Passes tests > Risk to taking this patch (and alternatives if risky): Risk is low to > medium. > The other option is disabling the ion optimizations for WebIDL bindings, > which > I suspect is riskier, not to mention a performance hit. Based on the risk evaluation here and keeping in mind the alternative is riskier along with perf impact approving the patch here. But a quick check on what sort of impact this may have in terms of new regressions? Are we talking about a possible stability issue ? > String or IDL/UUID changes made by this patch: None.

https://hg.mozilla.org/mozilla-central/rev/3e18a5b5257d https://hg.mozilla.org/mozilla-central/rev/db2babefda12

> But a quick check on what sort of impact this may have in terms of new regressions? The most likely fallout would be either correctness or stability issues with workers. Impact would be limited to pages that actually use workers. Given that we do pass our worker tests, and we have sane test coverage here, the most likely regression is actually another worker-related security-critical crash bug of some sort.... :(

https://hg.mozilla.org/releases/mozilla-aurora/rev/90661ab654d7 https://hg.mozilla.org/releases/mozilla-aurora/rev/95c8ca26e8bb