1018916 - Assertion failure: IsObjectValueInCompartment(value, compartment())

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Carsten Book [:Tomcat]]

Attachment: bughunter stack

found via bughunter on http://www.sherwin-williams.com/store-locator/

loading this site / or reloading cause

Assertion failure: IsObjectValueInCompartment(value, compartment()), at c:\work\mozilla\builds\nightly\mozilla\js\src\vm/ObjectImpl.h:731

Comment 1

[Boris Zbarsky [:bzbarsky]]

(gdb) frame 0
#0  js::ObjectImpl::setSlot (this=0x133a760c0, slot=1, value=@0x7fff5fbfbbd8) at ObjectImpl.h:731
731             MOZ_ASSERT(IsObjectValueInCompartment(value, compartment()));
(gdb) p ((nsGlobalWindow*)js::GetReservedSlot(this->compartment()->global_.value, 0).toPrivate())->mDoc.mRawPtr->mDocumentURI.mRawPtr->mSpec
$23 = {
  <nsACString_internal> = {
    mData = 0x124989b88 "http://www.sherwin-williams.com/store-locator/", 
    mLength = 46, 
    mFlags = 5
  }, <No data fields>}
(gdb) p ((nsGlobalWindow*)js::GetReservedSlot(value.toObjectOrNull()->compartment()->global_.value, 0).toPrivate())->mDoc.mRawPtr->mDocumentURI.mRawPtr->mSpec
$24 = {
  <nsACString_internal> = {
    mData = 0x124989b88 "http://www.sherwin-williams.com/store-locator/", 
    mLength = 46, 
    mFlags = 5
  }, <No data fields>}
(gdb) p this->compartment()
$25 = (class JSCompartment *) 0x12de7f000
(gdb) p value.toObjectOrNull()->compartment()
$26 = (class JSCompartment *) 0x11b614800

This is the source element stuff for scripts; not a JIT issue afaict.  Jim, why aren't we wrapping that into the right compartment?

Comment 2

[Carsten Book [:Tomcat]]

note that this is spread more, like also on http://www.qvc.de/LiveShowDisplay?langId=-3&storeId=10253&catalogId=10153&cm_re=70_Toolbox-_-70_3-_-QVC_Live_TV&cm_sp=70_Toolbox-_-70_3-_-QVC_Live_TV&sc=LIVE - a popular german homeshopping site

Comment 3

[Jim Blandy :jimb]

We're certainly supposed to be wrapping these things. Looking into it.

Comment 4

[Jim Blandy :jimb]

I am not able to reproduce this crash. But as far as I can tell, the owning element is supposed to be wrapped by this code:

https://hg.mozilla.org/mozilla-central/file/c90b38c47a1d/content/base/src/nsScriptLoader.cpp#l1091

  // XXXbz this is super-fragile, because the code that _uses_ the
  // JS::CompileOptions is nowhere near us, but we have to coordinate
  // compartments with it... and in particular, it will compile in the
  // compartment of aScopeChain, so we want to wrap into that compartment as
  // well.
  JSAutoCompartment ac(cx, aScopeChain);
  if (NS_SUCCEEDED(nsContentUtils::WrapNative(cx, aRequest->mElement,
                                              &elementVal,
                                              /* aAllowWrapping = */ true))) {
    MOZ_ASSERT(elementVal.isObject());
    aOptions->setElement(&elementVal.toObject());
  }

The crash is in an off-main-thread compilation. Is it possible that the current compartment of the JSContext passed to JS::FinishOffThreadScript might be different from the compartment of the JSContext passed to JS::CompileOffThread?

Comment 5

[Jim Blandy :jimb]

It would be possible to change the rules for CompileOptions: it could be SpiderMonkey's responsibility to properly wrap the element. Then, the above "super-fragile" code could be deleted.

Comment 6

[Jim Blandy :jimb]

Attachment: In off-thread compilation, rewrap the compilation options that might need it before saving them on the ScriptSourceObject.

I can't reproduce the crash myself, but this patch addresses my best guess as to the problem. If the try push looks clean, please see if the patch fixes the crash.

https://tbpl.mozilla.org/?tree=Try&rev=60588d4295ab

If it does fix the crash, then the patch should be expanded to cover on-thread compilation as well.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I can't reproduce the crash anymore either.  Either the site or something in our code changed...

Comment 8

[Jim Blandy :jimb]

I don't think it's actually sec-critical. It's a compartment violate, but the only thing that ever uses the field that would have the bad cross-compartment reference is Debugger; if the user never opens the devtools, this bug can't affect them.

Even if they do, Debugger will immediately wrap the bad cross-compartment edge in a Debugger.Object, which should be pretty safe. The devtools will just see a surprising reference.

Comment 9

[Jim Blandy :jimb]

I've filed bug 1031636 to simplify this code. Perhaps the nsScriptLoader.cpp snarl mentioned above can come out if it lands.

Comment 10

[Jim Blandy :jimb]

Now that I've landed bug 1031636, I would expect this not to occur any more. It's also no longer reproducible. Closing.

Comment 11

[Andrew McCreight [:mccr8]]

Bob Clary asked me to change this back to FIXED in email, so I will do that.  My interpretation of FIXED was that a fix was in the bug itself, but I guess that does not match the description.

Comment 12

[Matt Wobensmith [:mwobensmith][:matt:]]

Using URL in bug description:
Confirmed assert on Fx32, 2014-06-05.
Verified fixed on Fx33, 2014-10-06.