Closed Bug 736563 Opened 8 years ago Closed 8 years ago

Mark global objects black if they belong to an active window

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: billm, Assigned: mccr8)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [snappy])

Attachments

(1 file, 3 obsolete files)

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.
I can look at this next week, after I finish my current batch of CC micro-optimizations, if you'd like.
Assignee: nobody → continuation
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
Attached patch WIP, kind of hacky (obsolete) — Splinter Review
Minor variation on bug 735504.  This bug should obsolete that one, except for windows that are newly created in between GCs.
Though currently I only run that code once after a GC, so I suppose it is pretty redundant.
Blocks: 716598
Duplicate of this bug: 735504
This is crashy, but I think the problem is just that I need to null check what I pass to JS_CALL_OBJECT_TRACER.
Attached patch hopefully less crashy (obsolete) — Splinter Review
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
Attachment #606766 - Attachment is obsolete: true
Does nsGlobalWindow::GetWindowsTable(); really return only certainly alive windows?
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.
Ah, right. Windows with docshells should be alive.
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.
Also, on OSX, I don't get any Mochitest failures, whereas I got a number of them for Linux64, mostly in WebGL.
Attached patch even less bad (obsolete) — Splinter Review
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.
Attachment #606891 - Attachment is obsolete: true
Attached patch actually worksSplinter Review
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.
Attachment #607738 - Attachment is obsolete: true
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.
(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 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.
Attachment #608213 - Flags: review?(bugs)
Whiteboard: [snappy]
Blocks: 722715
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
Attachment #608213 - Flags: review?(bugs) → review+
review comments addressed, plus some other minor style fiddling.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb496ca24eef
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/bb496ca24eef
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.