Filed by: ncsoregi [at] mozilla.com https://treeherder.mozilla.org/logviewer.html#?job_id=166172071&repo=autoland https://queue.taskcluster.net/v1/task/BWjUVbwGTaajVnBv-8mNlg/runs/0/artifacts/public/logs/live_backing.log https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/BWjUVbwGTaajVnBv-8mNlg/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1 10:18:25 INFO - --DOMWINDOW == 2 (05E4E800) [pid = 3660] [serial = 2] [outer = 00000000] [url = about:blank] 10:24:35 INFO - REFTEST ERROR | file:///Z:/task_1520329263/build/tests/reftest/tests/layout/reftests/css-ui-valid/select/select-required-valid-changed-1.html | application timed out after 370 seconds with no output 10:24:35 INFO - REFTEST ERROR | Force-terminating active process(es). 10:24:35 INFO - REFTEST TEST-INFO | started process screenshot 10:24:36 INFO - REFTEST TEST-INFO | screenshot: exit 0 10:24:36 INFO - TEST-INFO | crashinject: exit 0 10:24:36 WARNING - TEST-UNEXPECTED-FAIL | file:///Z:/task_1520329263/build/tests/reftest/tests/layout/reftests/css-ui-valid/select/select-required-valid-changed-1.html | application terminated with exit code 1 10:24:36 INFO - REFTEST INFO | Copy/paste: Z:\task_1520329263\build\win32-minidump_stackwalk.exe c:\users\genericworker\appdata\local\temp\tmpx1v2gq.mozrunner\minidumps\649e4f95-9f8c-4688-ab06-a1c9319860ff.dmp Z:\task_1520329263\build\symbols 10:24:51 INFO - REFTEST INFO | Saved minidump as Z:\task_1520329263\build\blobber_upload_dir\649e4f95-9f8c-4688-ab06-a1c9319860ff.dmp 10:24:51 INFO - REFTEST INFO | Saved app info as Z:\task_1520329263\build\blobber_upload_dir\649e4f95-9f8c-4688-ab06-a1c9319860ff.extra 10:24:51 ERROR - REFTEST PROCESS-CRASH | file:///Z:/task_1520329263/build/tests/reftest/tests/layout/reftests/css-ui-valid/select/select-required-valid-changed-1.html | application crashed [@ js::SetProxyReservedSlot(JSObject *,unsigned int,JS::Value const &)] 10:24:51 INFO - Crash dump filename: c:\users\genericworker\appdata\local\temp\tmpx1v2gq.mozrunner\minidumps\649e4f95-9f8c-4688-ab06-a1c9319860ff.dmp

This one made me curious. I don't know enough to get to the bottom of it easily, but roughly what's happening is that we a a compartment->gcIncomingGrayPointers a->b->wrapper->tail1->tail2. We nuke 'wrapper' because we're closing a window, so we want to splice it out of the gcIncomingGrayPointers list. That means re-pointing 'b' to 'tail1', which asserts because 'b' is black and 'tail1' is gray. So I guess the question here is how 'b' can be black if it's in gcIncomingGrayPointers. It seems like things are only inserted into gcIncomingGrayPointers during gray marking and when TraceManuallyBarrieredCrossCompartmentEdge is used, and it seems like that only happens in Debugger trace hooks. Which seems to imply that the wrapper ought to be gray at insertion time, though I don't see an assertion for that. But assuming that's true, then it seems like the 'b' wrapper was blackened somehow after gray marking started? I would needinfo? jonco, but maybe I'll r? him on a diagnostic patch instead so he can tell me how I'm misunderstanding the whole setup.

Passed jit-tests and non262/, but I guess we do very very minimal gray marking there. Haven't tried the browser.

Heh. This does not make try very happy. https://treeherder.mozilla.org/#/jobs?repo=try&revision=5406eea5735582cfcd02ab7c56ce0b036978bc04&filter-tier=1 I haven't looked into these yet.

(In reply to Steve Fink [:sfink] [:s:] (PTO Apr9-12) from comment #6) > So I guess the question here is how 'b' can be black if it's in > gcIncomingGrayPointers. I think these can get unmarked gray during marking if we mark a wrapper to them black. > It seems like things are only inserted into > gcIncomingGrayPointers during gray marking and when > TraceManuallyBarrieredCrossCompartmentEdge is used This also happens from TraceCrossCompartmentEdge with is used for tracing proxy targets. > Heh. This does not make try very happy. Yeah, we don't exercise gray marking nearly enough during our tests. Actually we should add a test case for this particular situation.

Comment on attachment 8968631 [details] [diff] [review] diagnostic patch to detect non-gray entries in gcIncomingGrayPointers list Review of attachment 8968631 [details] [diff] [review]: ----------------------------------------------------------------- Cancelling review for now. ::: js/src/gc/GC.cpp @@ +5157,5 @@ > void > js::gc::DelayCrossCompartmentGrayMarking(JSObject* src) > { > MOZ_ASSERT(IsGrayListObject(src)); > + MOZ_ASSERT(src->isMarkedGray()); This should hold at least.

(In reply to Jon Coppeard (:jonco) from comment #9) > (In reply to Steve Fink [:sfink] [:s:] (PTO Apr9-12) from comment #6) > > So I guess the question here is how 'b' can be black if it's in > > gcIncomingGrayPointers. > > I think these can get unmarked gray during marking if we mark a wrapper to > them black. How does that work? The things in the list *are* cross-compartment wrappers. I didn't think we could have a CCW to a CCW -- if you wrap a CCW from some other compartment, you'll just get a CCW to the same target, unrelated to the other CCW. (I think there might be some exception, though...) So I'm not sure what you mean by marking a wrapper black. In the wrapper compartment, we've already moved on to gray marking. Ooh... or do we go back and forth between gray and black marking, now that the black/gray bit interpretation has changed? Do we do any gray unmarking after (the first time?) we start marking gray? Things like CellIter shouldn't, since you'd be in a trace session at that point. > > > It seems like things are only inserted into > > gcIncomingGrayPointers during gray marking and when > > TraceManuallyBarrieredCrossCompartmentEdge is used > > This also happens from TraceCrossCompartmentEdge with is used for tracing > proxy targets. Oops, sorry, I had figured that out but then submitted the comment without updating it. But still, it seems like it's during tracing. > > Heh. This does not make try very happy. > > Yeah, we don't exercise gray marking nearly enough during our tests. > Actually we should add a test case for this particular situation.

(In reply to Steve Fink [:sfink] [:s:] (PTO Apr9-12) from comment #11) > > I think these can get unmarked gray during marking if we mark a wrapper to > > them black. > > How does that work? My comment was badly phrased. I mean that the wrapper itself can be unmarked gray between the time we finish gray marking the wrapper compartment and the time we start gray marking the target compartment. This case is handled here by marking the targets of such wrappers black: https://searchfox.org/mozilla-central/source/js/src/gc/GC.cpp#5340 > Ooh... or do we go back > and forth between gray and black marking, now that the black/gray bit > interpretation has changed? Not yet, but we will do when we do incremental gray marking. > Do we do any gray unmarking after (the first time?) we start marking gray? Yes, any time we yield code can run and do gray unmarking.

It turned out that I'd bumped up against this when testing the browser with GC zeal and had a patch to remove this assertion. This case is expected as described above. These black->gray pointers do not outlive the GC as this list is unlinked during gray marking.

Comment on attachment 8969269 [details] [diff] [review] bug1443468-proxy-gray-checks Review of attachment 8969269 [details] [diff] [review]: ----------------------------------------------------------------- Did you want to fold the MOZ_ASSERT(src->isMarkedGray()) in MarkIncomingCrossCompartmentPointers into this patch?

Comment on attachment 8968631 [details] [diff] [review] diagnostic patch to detect non-gray entries in gcIncomingGrayPointers list Patch is invalid; we can do lots of stuff after adding things to the gcIncomingGrayPointers list, stuff that could unmark gray.

(In reply to Steve Fink [:sfink] [:s:] (PTO Apr9-12) from comment #15) The one in DelayCrossCompartmentGrayMarking? Sure.

Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/104deeaf58dc Suppress gray marking assertion during maniupulation of internal GC state r=sfink