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

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cbook, Assigned: jandem)

Tracking

(Blocks 1 bug, {assertion, regression, sec-high})

Trunk
mozilla51
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox48 unaffected, firefox49 unaffected, firefox-esr45 unaffected, firefox50 unaffected, firefox51- fixed)

Details

()

Attachments

(2 attachments)

Reporter

Description

3 years ago
Posted 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. 

STR:
-> Load https://shadow.com/shrc/zero_park?utm_source=Zero-Park&utm_medium=cpc&utm_campaign=POP%2B-%2BWorldwide&utm_content=romeo-gob-dSAuQk6D
--> Assertion failure 

seems its a trunk regression

marking s-s just in case
Reporter

Updated

3 years ago
Blocks: 532972
Reporter

Comment 1

3 years ago
[Tracking Requested - why for this release]:

trunk regression, bughunter found
Reporter

Comment 2

3 years ago
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
Assignee

Comment 6

3 years ago
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.
Assignee

Updated

3 years ago
Flags: needinfo?(jdemooij)
Assignee

Comment 7

3 years ago
Posted 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
Status: NEW → ASSIGNED
Attachment #8788546 - Flags: review?(jcoppeard)
Assignee

Comment 8

3 years ago
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]
Patch

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?
Assignee

Comment 10

3 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/2356dac937b7
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment on attachment 8788546 [details] [diff] [review]
Patch

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.
Depends on: 1304528
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.