Closed
Bug 1094531
Opened 10 years ago
Closed 10 years ago
Add CanSkip for nsNodeInfoManager
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: mccr8, Assigned: schien)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
6.25 KB,
patch
|
schien
:
review+
|
Details | Diff | Splinter Review |
Kyle suggested that nsNodeInfoManager could be skippable until DropDocumentReference() is called.
Assignee: nobody → schien
Comment 1•10 years ago
|
||
Hmm, how. We have for example document->rootElement->nodeinfo->nodeinfomanager->document cycle. DropDocumentReference() is called in document's dtor. I guess we could skip if document itself is skippable. So effectively remove if (tmp->mDocument && nsCCUncollectableMarker::InGeneration(cb, tmp->mDocument->GetMarkedCCGeneration())) { return NS_SUCCESS_INTERRUPTED_TRAVERSE; } and use canSkip.
Yeah, I was just guessing, but I was wrong. Checking the skippability of the document itself is the way to go.
Assignee | ||
Comment 3•10 years ago
|
||
Compilable patch based on comment #1 but haven't test it.
Reporter | ||
Comment 4•10 years ago
|
||
Thanks for working on this. The CAN_SKIP stuff is actually declared for a method for an inner class of nsNodeInfoManager so you don't need the helper methods, because they can access private fields directly. Also, you probably want to leave in the null checks on mDocument.
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8519389 -
Attachment is obsolete: true
Attachment #8520173 -
Flags: review?(bugs)
Assignee | ||
Comment 6•10 years ago
|
||
Address Andrew's suggestion in comment #4. try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b2e07c5c66a4
Attachment #8520173 -
Attachment is obsolete: true
Attachment #8520173 -
Flags: review?(bugs)
Attachment #8520192 -
Flags: review?(bugs)
Comment 7•10 years ago
|
||
Comment on attachment 8520192 [details] [diff] [review] make-node-info-manager-skippable.patch >+NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN(nsNodeInfoManager) >+ if (!tmp->mDocument) { >+ return false; >+ } >+ >+ return NS_CYCLE_COLLECTION_PARTICIPANT(nsDocument)->CanSkip(tmp->mDocument, aRemovingAllowed); >+NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_END >+ >+NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_BEGIN(nsNodeInfoManager) >+ if (!tmp->mDocument) { >+ return false; >+ } >+ >+ return NS_CYCLE_COLLECTION_PARTICIPANT(nsDocument)->CanSkipInCC(tmp->mDocument); >+NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_END >+ >+// CanSkipThis returns false to avoid problems with incomplete unlinking. >+NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_THIS_BEGIN(nsNodeInfoManager) >+ if (!tmp->mDocument) { >+ return false; >+ } >+ >+ return NS_CYCLE_COLLECTION_PARTICIPANT(nsDocument)->CanSkipThis(tmp->mDocument); >+NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_THIS_END I would write all these NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_... if (tmp->mDocument) { return NS_CYCLE_COLLECTION_PARTICIPANT(nsDocument)->{the right variant of canskip}(tmp->mDocument); } NS_IMPL_CYCLE_COLLECTION_CAN_SKIP... since the _END macros return false anyway. // CanSkipThis returns false to avoid problems with incomplete unlinking. I don't understand this comment. As far as I see, you could just drop it.
Attachment #8520192 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #7) > Comment on attachment 8520192 [details] [diff] [review] > make-node-info-manager-skippable.patch > > >+NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN(nsNodeInfoManager) > >+ if (!tmp->mDocument) { > >+ return false; > >+ } > >+ > >+ return NS_CYCLE_COLLECTION_PARTICIPANT(nsDocument)->CanSkip(tmp->mDocument, aRemovingAllowed); > >+NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_END > >+ > >+NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_BEGIN(nsNodeInfoManager) > >+ if (!tmp->mDocument) { > >+ return false; > >+ } > >+ > >+ return NS_CYCLE_COLLECTION_PARTICIPANT(nsDocument)->CanSkipInCC(tmp->mDocument); > >+NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_END > >+ > >+// CanSkipThis returns false to avoid problems with incomplete unlinking. > >+NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_THIS_BEGIN(nsNodeInfoManager) > >+ if (!tmp->mDocument) { > >+ return false; > >+ } > >+ > >+ return NS_CYCLE_COLLECTION_PARTICIPANT(nsDocument)->CanSkipThis(tmp->mDocument); > >+NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_THIS_END > > > > I would write all these > > NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_... > if (tmp->mDocument) { > return NS_CYCLE_COLLECTION_PARTICIPANT(nsDocument)->{the right variant > of canskip}(tmp->mDocument); > } > NS_IMPL_CYCLE_COLLECTION_CAN_SKIP... > > since the _END macros return false anyway. Will do. > > > // CanSkipThis returns false to avoid problems with incomplete unlinking. > I don't understand this comment. As far as I see, you could just drop it. I forgot to remove this comment after copying these macros from other places. Will remove it in the final patch.
Assignee | ||
Comment 9•10 years ago
|
||
forgot to upload my latest patch, carry r+. try looks good and ready to land. try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d41c9aba6362
Attachment #8520192 -
Attachment is obsolete: true
Attachment #8524068 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/39b1bc270aed
Keywords: checkin-needed
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/39b1bc270aed
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•