Mark global objects black if they belong to an active window

RESOLVED FIXED in mozilla14

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: billm, Assigned: mccr8)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [snappy])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

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

Comment 2

5 years ago
Cool!
(Assignee)

Updated

5 years ago
Assignee: nobody → continuation
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
Though currently I only run that code once after a GC, so I suppose it is pretty redundant.
(Assignee)

Updated

5 years ago
Blocks: 716598
(Assignee)

Updated

5 years ago
Duplicate of this bug: 735504
(Assignee)

Comment 6

5 years ago
This is crashy, but I think the problem is just that I need to null check what I pass to JS_CALL_OBJECT_TRACER.
(Assignee)

Comment 7

5 years ago
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
Attachment #606766 - Attachment is obsolete: true

Comment 8

5 years ago
Does nsGlobalWindow::GetWindowsTable(); really return only certainly alive windows?
(Assignee)

Comment 9

5 years ago
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.
(Assignee)

Comment 11

5 years ago
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.
(Assignee)

Comment 12

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

Comment 13

5 years ago
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.
Attachment #606891 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
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.
Attachment #607738 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
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.
(Reporter)

Comment 16

5 years ago
(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.
(Assignee)

Comment 17

5 years ago
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]
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 19

5 years ago
review comments addressed, plus some other minor style fiddling.

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

Updated

5 years ago
Target Milestone: --- → mozilla14
(Assignee)

Comment 20

5 years ago
https://hg.mozilla.org/mozilla-central/rev/bb496ca24eef
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.