Closed Bug 1300211 Opened 3 years ago Closed 3 years ago

Black marking in XULDocument is a bit weird

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: bzbarsky, Assigned: mccr8)

Details

Attachments

(1 file)

Consider a GC that happens while a XULDocument has mCurrentPrototype set but has _not_ yet moved it over into its mPrototypes list.  mozilla::dom::TraceBlackJS will call XULDocument::TraceProtos which will mark black all scripts in the mPrototypes list.  Then we will mark the things in mCurrentPrototype gray.  Then at some point we will append it to the mPrototypes list, at which point we now have a bunch of black roots that are actually marked gray.  This seems like a violation of our black-gray invariant to me.
Arguably, XULDocument::TraceProtos should just trace mCurrentPrototype if that's not null and not equal to the last thing in mPrototypes.
Flags: needinfo?(continuation)
It seems to me that we could just mark mCurrentPrototype black, too. There's no harm in marking it twice if it in mPrototypes already. I didn't audit if there are any other gray -> black transitions that could happen.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7c9b26f2e2b
Flags: needinfo?(continuation)
Assignee: nobody → continuation
No harm except that black marking is not terribly fast....
Adding a root twice should cost almost no time. The second time we add it as a root the GC should just see that the object is already marked and not do any further work.
Yes, but nsXULPrototypeDocument::TraceProtos walks over the whole proto node tree looking for things to add as roots.

On the other hand, it also optimizes based on gc number, so maybe it's ok.
Comment on attachment 8789923 [details]
Bug 1300211 - Trace XULDocument::mCurrentPrototype.

https://reviewboard.mozilla.org/r/77964/#review76494
Attachment #8789923 - Flags: review?(bugs) → review+
(In reply to Boris Zbarsky [:bz] (busy, pun intended) from comment #6)
> Yes, but nsXULPrototypeDocument::TraceProtos walks over the whole proto node
> tree looking for things to add as roots.

Oh, oops, right. I always forget that that method is complex.
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d41128490bb7
Trace XULDocument::mCurrentPrototype. r=smaug
https://hg.mozilla.org/mozilla-central/rev/d41128490bb7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.