Add CanSkip for nsNodeInfoManager

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 months ago

People

(Reporter: mccr8, Assigned: schien)

Tracking

(Blocks 1 bug)

Trunk
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Kyle suggested that nsNodeInfoManager could be skippable until DropDocumentReference() is called.
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.
Blocks: 869187
Compilable patch based on comment #1 but haven't test it.
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.
Attachment #8519389 - Attachment is obsolete: true
Attachment #8520173 - Flags: review?(bugs)
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 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+
(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.
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+
https://hg.mozilla.org/mozilla-central/rev/39b1bc270aed
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.