Closed Bug 1018916 Opened 6 years ago Closed 6 years ago

Assertion failure: IsObjectValueInCompartment(value, compartment())

Categories

(Core :: JavaScript Engine, defect)

x86
All
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox33 --- verified
firefox-esr24 --- wontfix
firefox-esr31 --- wontfix

People

(Reporter: cbook, Assigned: jimb)

References

(Blocks 1 open bug, )

Details

(Keywords: assertion, sec-moderate, Whiteboard: [adv-main33+])

Attachments

(2 files)

Attached file 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
(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?
Component: JavaScript Engine: JIT → JavaScript Engine
Flags: needinfo?(jimb)
Keywords: sec-high
We're certainly supposed to be wrapping these things. Looking into it.
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?
Flags: needinfo?(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.
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.
I can't reproduce the crash anymore either.  Either the site or something in our code changed...
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.
I've filed bug 1031636 to simplify this code. Perhaps the nsScriptLoader.cpp snarl mentioned above can come out if it lands.
Now that I've landed bug 1031636, I would expect this not to occur any more. It's also no longer reproducible. Closing.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Resolution: FIXED → WORKSFORME
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.
Resolution: WORKSFORME → FIXED
Assignee: nobody → jimb
Target Milestone: --- → mozilla33
Whiteboard: [adv-main33+]
Using URL in bug description:
Confirmed assert on Fx32, 2014-06-05.
Verified fixed on Fx33, 2014-10-06.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.