Last Comment Bug 736563 - Mark global objects black if they belong to an active window
: Mark global objects black if they belong to an active window
Status: RESOLVED FIXED
[snappy]
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Andrew McCreight [:mccr8]
:
: Andrew Overholt [:overholt]
Mentors:
: 735504 (view as bug list)
Depends on:
Blocks: 716598 722715
  Show dependency treegraph
 
Reported: 2012-03-16 12:15 PDT by [PTO to Dec5] Bill McCloskey (:billm)
Modified: 2012-03-28 08:06 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP, kind of hacky (3.19 KB, patch)
2012-03-16 15:36 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
hopefully less crashy (3.29 KB, patch)
2012-03-17 11:45 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
even less bad (3.93 KB, patch)
2012-03-20 15:13 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
actually works (3.42 KB, patch)
2012-03-21 20:50 PDT, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review

Description [PTO to Dec5] Bill McCloskey (:billm) 2012-03-16 12:15:57 PDT
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] 2012-03-16 12:17:28 PDT
I can look at this next week, after I finish my current batch of CC micro-optimizations, if you'd like.
Comment 2 [PTO to Dec5] Bill McCloskey (:billm) 2012-03-16 12:19:10 PDT
Cool!
Comment 3 Andrew McCreight [:mccr8] 2012-03-16 15:36:37 PDT
Created attachment 606766 [details] [diff] [review]
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] 2012-03-16 15:40:04 PDT
Though currently I only run that code once after a GC, so I suppose it is pretty redundant.
Comment 5 Andrew McCreight [:mccr8] 2012-03-16 17:05:57 PDT
*** Bug 735504 has been marked as a duplicate of this bug. ***
Comment 6 Andrew McCreight [:mccr8] 2012-03-17 08:40:07 PDT
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] 2012-03-17 11:45:58 PDT
Created attachment 606891 [details] [diff] [review]
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] 2012-03-18 07:11:21 PDT
Does nsGlobalWindow::GetWindowsTable(); really return only certainly alive windows?
Comment 9 Andrew McCreight [:mccr8] 2012-03-18 07:16:32 PDT
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] 2012-03-18 08:05:41 PDT
Ah, right. Windows with docshells should be alive.
Comment 11 Andrew McCreight [:mccr8] 2012-03-20 10:52:18 PDT
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] 2012-03-20 10:53:53 PDT
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] 2012-03-20 15:13:18 PDT
Created attachment 607738 [details] [diff] [review]
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] 2012-03-21 20:50:43 PDT
Created attachment 608213 [details] [diff] [review]
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] 2012-03-21 21:38:32 PDT
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 [PTO to Dec5] Bill McCloskey (:billm) 2012-03-21 21:40:20 PDT
(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] 2012-03-22 07:47:38 PDT
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] 2012-03-26 16:45:31 PDT
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] 2012-03-27 12:23:52 PDT
review comments addressed, plus some other minor style fiddling.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb496ca24eef
Comment 20 Andrew McCreight [:mccr8] 2012-03-28 08:06:32 PDT
https://hg.mozilla.org/mozilla-central/rev/bb496ca24eef

Note You need to log in before you can comment on or make changes to this bug.