736563 - Mark global objects black if they belong to an active window

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Andrew said this shouldn't be too hard. During the XPConnect black marking hook, we would iterate over all windows, check if the window is active, find the context for the window, and then mark its global object black.

Comment 1

[Andrew McCreight [:mccr8]]

I can look at this next week, after I finish my current batch of CC micro-optimizations, if you'd like.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Cool!

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: WIP, kind of hacky

Minor variation on bug 735504.  This bug should obsolete that one, except for windows that are newly created in between GCs.

Comment 4

[Andrew McCreight [:mccr8]]

Though currently I only run that code once after a GC, so I suppose it is pretty redundant.

Comment 6

[Andrew McCreight [:mccr8]]

This is crashy, but I think the problem is just that I need to null check what I pass to JS_CALL_OBJECT_TRACER.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: hopefully less crashy

I added the null check, and renamed the function.  Ideally, we'd convert all of the cleanupJS functionality to TRACE_JS instead of unmark gray, and then move it into this function, so I wanted to give it a more generally name.

https://tbpl.mozilla.org/?tree=Try&rev=b29f541080f2

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Does nsGlobalWindow::GetWindowsTable(); really return only certainly alive windows?

Comment 9

[Andrew McCreight [:mccr8]]

No, it returns them all, so I filter out those that don't have doc shells.  I'm following the logic listed in the comment in CollectWindowReports that is used for about:memory:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsWindowMemoryReporter.cpp#92

The code there implies that windows with doc shells must be "active" ("currently either displayed in an active tab, or a child of such a window") or in the fastback cache, if they have doc shells.  Maybe I'm wrong about that being the right thing to do.  It is still pretty crashy, so something isn't right.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Ah, right. Windows with docshells should be alive.

Comment 11

[Andrew McCreight [:mccr8]]

recursion.js fails locally when run as part of the suite, which is what I saw on try, but passes when run by itself, or even in somewhat larger chunks of tests.  Well, first it hangs on a test that is showing up as a random orange on TBPL (I don't have the bug number handy), but if I disable that then I get the recursion failures.

Comment 12

[Andrew McCreight [:mccr8]]

Also, on OSX, I don't get any Mochitest failures, whereas I got a number of them for Linux64, mostly in WebGL.

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: even less bad

Bill suggested that I check that every object that I was marking black was the object of a context we were going to mark gray.  It turns out that this was not the case.  From mrbkap in IRC, I found out that the global object of an inner window is not the same as the global object of its context (but is the same for the outer window).  I don't know why, but grabbing the global from the context seems to at least not trigger the assertion on a test case that was breaking before.  I'm not sure if having this debug code is worthwhile to actually land, as I was just making a silly mistake.  I'm not sure what broke exactly, but maybe the JS object was dead or something.

Comment 14

[Andrew McCreight [:mccr8]]

Attachment: actually works

Getting the global object out of the context takes an annoying number of null checks (which I failed to do in the previous version of my patch), so I went with this approach, where we instead grab the global object directly from the window, but only for outer windows.  A slightly earlier version of this was causing a random orange to happen much more frequently, but it seems okay now.  Either the check I added to disable the optimization on shutdown fixed it, or updating my tree fixed it.  I just need to confirm that removing some debugging code I added won't mess things up.

Comment 15

[Andrew McCreight [:mccr8]]

I didn't notice any particular speed difference on a simple test case I tried, and it didn't actually help with the test case in bug 717500 unfortunately.  It did get rid of those annoying ChromeWindow JS blobs I was seeing in the CC graph.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Andrew McCreight [:mccr8] from comment #15)
> I didn't notice any particular speed difference on a simple test case I
> tried, and it didn't actually help with the test case in bug 717500
> unfortunately.  It did get rid of those annoying ChromeWindow JS blobs I was
> seeing in the CC graph.

Nevertheless, this patch should at least give us some peace of mind when we start calling UnmarkGray more often.

Comment 17

[Andrew McCreight [:mccr8]]

Comment on attachment 608213 [details] [diff] [review]
actually works

Try run looked good.  I'm open to suggestions regarding the name and location of dom::TraceBlackJS.  I basically went with the least creative possible options.  I removed the debugging code I added that checks that each object you mark is the global object of a context, but I could add that back in if it makes sense.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 608213 [details] [diff] [review]
actually works

>+mozilla::dom::TraceBlackJS(JSTracer *trc)
>+{
>+  if (!nsCCUncollectableMarker::sGeneration)
>+    return;
if (expr) {
  stmt;
}

>+
>+  // Mark globals of active windows black.
>+  nsGlobalWindow::WindowByIdTable* windowsById =
>+    nsGlobalWindow::GetWindowsTable();
>+  if (windowsById)
>+    windowsById->Enumerate(TraceActiveWindowGlobal, trc);
Ditto

>+void TraceBlackJS(JSTracer* trc);
aParameterName

Comment 19

[Andrew McCreight [:mccr8]]

review comments addressed, plus some other minor style fiddling.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb496ca24eef

Comment 20

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/mozilla-central/rev/bb496ca24eef