Closed Bug 1300548 Opened 5 years ago Closed 5 years ago

Assertion failure: obj->compartment() == cx->compartment(), at VMFunctions.cpp:1171


(Core :: JavaScript Engine: JIT, defect)

Not set



Tracking Status
firefox48 --- unaffected
firefox49 --- unaffected
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 - fixed


(Reporter: cbook, Assigned: jandem)




(Keywords: assertion, regression, sec-high)


(2 files)

Attached file complete stack
Assertion failure: obj->compartment() == cx->compartment(), at c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/js/src/jit/VMFunctions.cpp:1171

found by bughunter and reproduced on latest windows trunk debug tinderbox build. 

-> Load
--> Assertion failure 

seems its a trunk regression

marking s-s just in case
[Tracking Requested - why for this release]:

trunk regression, bughunter found
ryan would you be able to find someone who can check for a regression range ?
Flags: needinfo?(ryanvm)
I'm having a tough time getting this to reliably reproduce. I hit it once via mozregression on a 2016-09-04 build, but wasn't able to hit it again. I've got a bad feeling this is ad-related :(
Flags: needinfo?(ryanvm)
OK, reloading a few times appears to be enough to hit it eventually on affected builds. Still a pain to reproduce consistently, but I've got it down a range that includes bug 1295967, which at least seems plausible. WDYT, Jan?
Yeah, I'm certain it's from bug 1295967. The revision prior to it landing (4f4a7f9d6e59) doesn't reproduce no matter how many times I reload.
Blocks: 1295967
Flags: in-testsuite?
Version: unspecified → Trunk
Interesting assertion failure. Might be from bug 1295967, or it exposed this somehow.

I can reproduce a crash on OS X, will try on Linux as well so I can get an rr recording.
Flags: needinfo?(jdemooij)
Attached patch PatchSplinter Review
The native iterator cache was based on the Shape, and now that shapes are shared within a Zone, we could hand out iterator objects from different compartments. Oops.

The patch fixes this as follows:

* The NativeIterCache::last pointer I moved to JSCompartment. This one is accessed by JIT code and that path is fairly hot, so moving it to the compartment means we don't need an extra guard.

* We do a compartment check for iterators we get from NativeIterCache.
Assignee: nobody → jdemooij
Attachment #8788546 - Flags: review?(jcoppeard)
Tomcat and Ryan, thanks for filing and bisecting this. Very nice find.
Group: core-security → javascript-core-security
Keywords: sec-high
Comment on attachment 8788546 [details] [diff] [review]

Review of attachment 8788546 [details] [diff] [review]:

::: js/src/jscompartment.cpp
@@ +859,5 @@
>  void
>  JSCompartment::fixupAfterMovingGC()
>  {
> +    purge();

Don't we need to clear the pointer when sweeping and not just when we do a moving GC?
(In reply to Jon Coppeard (:jonco) from comment #9)
> Don't we need to clear the pointer when sweeping and not just when we do a
> moving GC?

We also call comp->purge() in GCRuntime::purgeRuntime.
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment on attachment 8788546 [details] [diff] [review]

Review of attachment 8788546 [details] [diff] [review]:

Indeed, looks good then!
Attachment #8788546 - Flags: review?(jcoppeard) → review+
Not going to track for 51 since it is now fixed.
Group: javascript-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.