Closed Bug 815999 Opened 7 years ago Closed 7 years ago

Intermittent Assertion failure: !dst->compartment()->isCollecting() in browser_dbg_chrome-debugging.js [@ EndMarkingCompartmentGroup] (now [@ ShouldMarkCrossCompartment])

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: peterv, Assigned: jonco)

References

Details

(Keywords: assertion, intermittent-failure)

Crash Data

Attachments

(3 files, 2 obsolete files)

https://tbpl.mozilla.org/php/getParsedLog.php?id=17399462&tree=Mozilla-Inbound
Rev5 MacOSX Mountain Lion 10.8 mozilla-inbound debug test mochitest-browser-chrome on 2012-11-27 21:41:59 PST for push 6f0c997ba776
slave: talos-mtnlion-r5-077

TEST-START | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_chrome-debugging.js
++DOCSHELL 0x158da7e60 == 13 [id = 2758]
++DOMWINDOW == 54 (0x1281cba08) [serial = 7287] [outer = 0x0]
++DOMWINDOW == 55 (0x158d872a8) [serial = 7288] [outer = 0x1281cb980]
WARNING: NS_ENSURE_TRUE(mMutable) failed: file ../../../../netwerk/base/src/nsSimpleURI.cpp, line 272
WARNING: NS_ENSURE_TRUE(mMutable) failed: file ../../../../netwerk/base/src/nsSimpleURI.cpp, line 272
WARNING: NS_ENSURE_TRUE(frame) failed: file ../../../layout/base/nsPresContext.cpp, line 1179
WARNING: NS_ENSURE_TRUE(frame) failed: file ../../../layout/base/nsPresContext.cpp, line 1179
WARNING: NS_ENSURE_TRUE(frame) failed: file ../../../layout/base/nsPresContext.cpp, line 1179
++DOMWINDOW == 56 (0x150387478) [serial = 7289] [outer = 0x1281cb980]
WARNING: Unable to test style tree integrity -- no content node: file ../../../layout/base/nsCSSFrameConstructor.cpp, line 8267
TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_chrome-debugging.js | Found a chrome debugging actor.
Assertion failure: !dst->compartment()->isCollecting(), at ../../../js/src/jsgc.cpp:4010
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_chrome-debugging.js | Exited with code 1 during test run
INFO | automation.py | Application ran for: 0:21:54.527508
INFO | automation.py | Reading PID log: /var/folders/7c/fqxs206n3dlcgjphjlbqjk6000000w/T/tmpnKkKJTpidlog
Downloading symbols from: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-macosx64-debug/1354079312/firefox-20.0a1.en-US.mac64.crashreporter-symbols.zip
PROCESS-CRASH | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_chrome-debugging.js | application crashed (minidump found)
Crash dump filename: /var/folders/7c/fqxs206n3dlcgjphjlbqjk6000000w/T/tmpCIIQ8L/minidumps/D79E9359-EC94-4171-8DAC-1F1899DEDFB4.dmp
Operating system: Mac OS X
                  10.8.0 12A269
CPU: amd64
     family 6 model 42 stepping 7
     8 CPUs

Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash address: 0x0

Thread 0 (crashed)
 0  XUL!EndMarkingCompartmentGroup [jsgcinlines.h : 460 + 0x0]
    rbx = 0x00007fff7eb98c68   r12 = 0x000000016d272800
    r13 = 0x00007fff5fbf65c0   r14 = 0x0000000104c8a000
    r15 = 0x00007fff5fbf65b8   rip = 0x000000010302f64e
    rsp = 0x00007fff5fbf6580   rbp = 0x00007fff5fbfa640
    Found by: given as instruction pointer in context
 1  XUL!IncrementalCollectSlice [jsgc.cpp : 4206 + 0x7]
    rbx = 0x00007fff5fbfa758   r12 = 0x0000000104c8a000
    r13 = 0x0000000103884be0   r14 = 0x00007fff5fbfa690
    r15 = 0x0000000104c8a000   rip = 0x000000010302c414
    rsp = 0x00007fff5fbfa650   rbp = 0x00007fff5fbfa7d0
    Found by: call frame info
 2  XUL!ResetIncrementalGC [jsgc.cpp : 4464 + 0x10]
    rbx = 0x0000000104c8a000   r12 = 0x0000000000000000
    r13 = 0x0000000000000000   r14 = 0x000000010369b4f6
    r15 = 0x0000000104c8a000   rip = 0x000000010302b9a5
    rsp = 0x00007fff5fbfa7e0   rbp = 0x00007fff5fbfa820
    Found by: call frame info
 3  XUL!GCCycle [jsgc.cpp : 4809 + 0x11]
    rbx = 0x0000000104c8a000   r12 = 0x0000000000000000
    r13 = 0x0000000000000000   r14 = 0x000000010369b4f6
    r15 = 0x0000000104c8a000   rip = 0x000000010302b68a
    rsp = 0x00007fff5fbfa830   rbp = 0x00007fff5fbfa870
    Found by: call frame info
 4  XUL!Collect [jsgc.cpp : 4931 + 0x14]
    rbx = 0x0000000104c8a000   r12 = 0x0000000000000000
    r13 = 0x0000000104c8a3f8   r14 = 0x0000000000000008
    r15 = 0x0000000000000000   rip = 0x000000010302820c
    rsp = 0x00007fff5fbfa880   rbp = 0x00007fff5fbfa8c0
    Found by: call frame info
 5  XUL!JSCompartment::addDebuggee(JSContext*, js::GlobalObject*) [jscompartment.h : 523 + 0xf]
    rbx = 0x000000004c320aac   r12 = 0x0000000100841e00
    r13 = 0x0000000000000000   r14 = 0x0000000100174f90
    r15 = 0x0000000100842501   rip = 0x0000000102ff449a
    rsp = 0x00007fff5fbfa8d0   rbp = 0x00007fff5fbfa930
    Found by: call frame info
 6  XUL!js::Debugger::addDebuggeeGlobal(JSContext*, JS::Handle<js::GlobalObject*>) [Debugger.cpp : 2089 + 0xe]
    rbx = 0x00007fff5fbfaa40   r12 = 0x0000000100174f90
    r13 = 0x00007fff5fbfaa40   r14 = 0x000000016e999360
    r15 = 0x00000001503505d0   rip = 0x00000001031a3599
    rsp = 0x00007fff5fbfa940   rbp = 0x00007fff5fbfaa20
    Found by: call frame info
 7  XUL!js::Debugger::addDebuggee(JSContext*, unsigned int, JS::Value*) [Debugger.cpp : 1878 + 0xa]
    rbx = 0x00000001503505d0   r12 = 0x0000000100174f90
    r13 = 0x00007fff5fbfaab8   r14 = 0x000000010cc97310
    r15 = 0x0000000000000000   rip = 0x00000001031a2f47
    rsp = 0x00007fff5fbfaa30   rbp = 0x00007fff5fbfaa90
    Found by: call frame info
 8  XUL!js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [jscntxtinlines.h : 364 + 0x5]
    rbx = 0x000000010cc97318   r12 = 0x00007fff5fbfabd0
    r13 = 0x00007fff5fbfaab8   r14 = 0x0000000100174f90
    r15 = 0x00000001031a2ec0   rip = 0x000000010306ee1c
    rsp = 0x00007fff5fbfaaa0   rbp = 0x00007fff5fbfaaf0
    Found by: call frame info
Summary: Intermittent Assertion failure: !thing->compartment()->scheduledForDestruction in browser_dbg_chrome-debugging.js [@ EndMarkingCompartmentGroup] → Intermittent Assertion failure: dst->compartment()->isCollecting() in browser_dbg_chrome-debugging.js [@ EndMarkingCompartmentGroup]
Summary: Intermittent Assertion failure: dst->compartment()->isCollecting() in browser_dbg_chrome-debugging.js [@ EndMarkingCompartmentGroup] → Intermittent Assertion failure: !dst->compartment()->isCollecting() in browser_dbg_chrome-debugging.js [@ EndMarkingCompartmentGroup]
Assignee: general → jcoppeard
Status: NEW → ASSIGNED
Attached patch Possible fix (obsolete) — Splinter Review
Change is to always sweep debugger and debugee objects in the same group, as I think was the original intention.
The original assertion assumes that a compartment edge from A -> B means that A will not finish marking after B.  However, the order is calculated at the start of the sweep phase, and new wrappers can be created after this point which break this assumption.

The fix is to update the assertion to ignore wrappers created during collection.

This necessitates moving the finding of black->gray into the marking code, because the areana's allocatedDuringIncremental flag is cleared after the arena has been marked.  This also means that it's no longer necessary to iterate though every wrapper to check this.

Also included is the previous patch that ensures debugger and debuggee objects are always finalized in the same group.
Attachment #686133 - Attachment is obsolete: true
Attachment #686562 - Flags: review?(wmccloskey)
As discussed with billm, this is a quick patch to disallow
Attachment #686652 - Flags: review?(wmccloskey)
... to disallow creation of wrappers to gray objects, to see if we can get to the root of this problem.

We could probably do with factoring out all calls to JSCompartment::crossCompartmentWrappers.put() but I haven't done that yet.
Comment on attachment 686652 [details] [diff] [review]
Patch to assert if a wrapper to a gray thing is created

I can check this in for you later today, once the try build finishes.
Attachment #686652 - Flags: review?(wmccloskey) → review+
The try build is pretty revealing. I think that findAllGlobals and RecomputeWrappers need to call UnmarkGray. That means we'll have to move it to the JS engine. I'll work on that tomorrow. That should be enough to fix this problem, I suspect.
Comment on attachment 686562 [details] [diff] [review]
Better potential fix

Review of attachment 686562 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/gc/Marking.cpp
@@ +603,5 @@
> +         * compartment that has already been marked gray.
> +         */
> +        if (cell->isMarked(GRAY)) {
> +            JS_ASSERT_IF(!src->arenaHeader()->allocatedDuringIncremental,
> +                         !cell->compartment()->isCollecting());

Please remove the allocatedDuringIncremental condition.
Attachment #686562 - Flags: review?(wmccloskey) → review+
Whiteboard: [leave open]
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc50a2127738

Not a fix, just a couple of improvements:
 - sweep debugger and debugee objects in same group 
 - improve black->gray pointer detection
This caused crashes in some of the unit tests, so I backed it out.

https://hg.mozilla.org/integration/mozilla-inbound/rev/70e354775e1b
(In reply to Ehsan Akhgari [:ehsan] from comment #39)
> This caused crashes in some of the unit tests, so I backed it out.

Thanks.  Can you point me at the tests that crashed?
I pushed this to try again and got a green build, so I'm re-landing it: https://tbpl.mozilla.org/?tree=Try&rev=0bad9e4bc457

I believe there was another GC-related patch that later got reverted that could have caused the failures.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7f71ff116c3
(In reply to comment #45)
> (In reply to Ehsan Akhgari [:ehsan] from comment #39)
> > This caused crashes in some of the unit tests, so I backed it out.
> 
> Thanks.  Can you point me at the tests that crashed?

Sorry I didn't put the links in the bug.  FWIW you can go to the TBPL page for the inbound tree, and click the green "next" button at the end of the page enough number of times to get to your push the next time.
The problem here is that Debugger::findAllGlobals plucks global objects directly out of the compartment. There's no assurance that these globals are marked black--they could be gray. Then it exposes them to JS code.
Attachment #688062 - Flags: review?(continuation)
Forgot qref.
Attachment #688062 - Attachment is obsolete: true
Attachment #688062 - Flags: review?(continuation)
Attachment #688063 - Flags: review?(continuation)
Comment on attachment 688063 [details] [diff] [review]
fix for black-to-gray edge from debugger, v2

Review of attachment 688063 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/HeapAPI.h
@@ +78,5 @@
> + * engine (by handing it to running JS code or writing it into live JS
> + * data). During incremental GC, since the gray bits haven't been computed yet,
> + * we conservatively mark the object black.
> + */
> +static inline void

This should be MOZ_ALWAYS_INLINE.

@@ +79,5 @@
> + * data). During incremental GC, since the gray bits haven't been computed yet,
> + * we conservatively mark the object black.
> + */
> +static inline void
> +ExposeGCThingToJS(void *thing, JSGCTraceKind kind)

This name is slightly weird. Aren't JS objects always exposed to JS? I mean, I know what you mean, but maybe something like ExposeGCThingToLiveJS?  ToActiveJS? etc.  Well, "live" isn't that great either, given that gray JS is still alive...

::: js/src/jsfriendapi.cpp
@@ +893,5 @@
>      return IsIncrementalBarrierNeeded(cx->runtime);
>  }
>  
>  JS_FRIEND_API(bool)
> +js::IsIncrementalBarrierNeededOnGCThing(void *thing, JSGCTraceKind kind)

Why do you have this |kind| argument you never use?

::: js/src/jsfriendapi.h
@@ -11,5 @@
>  #include "jscpucfg.h"
>  #include "jspubtd.h"
>  #include "jsprvtd.h"
>  
> -#include "js/HeapAPI.h"

I hope you checked that this compiles. ;)
Attachment #688063 - Flags: review?(continuation) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/97bdf3a66177

I've been trying to pass around the thing kind more often just because we'll probably need it for generational collection.
Crash Signature: [@ EndMarkingCompartmentGroup] → [@ EndMarkingCompartmentGroup] [@ ShouldMarkCrossCompartment]
Summary: Intermittent Assertion failure: !dst->compartment()->isCollecting() in browser_dbg_chrome-debugging.js [@ EndMarkingCompartmentGroup] → Intermittent Assertion failure: !dst->compartment()->isCollecting() in browser_dbg_chrome-debugging.js [@ EndMarkingCompartmentGroup] (now [@ ShouldMarkCrossCompartment])
This looks fixed.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.