1974513 - TextDirectiveFinder and TextDirectiveCreator should use ref pointers for mDocument

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

TextDirectiveFinder and TextDirectiveCreator effectively use raw pointers for their mDocument fields. This is very dodgy because unless the classes are used very carefully this could lead to a use-after-free. I'm pretty sure they are actually okay because they are used in a stacklike way, although they are heap allocated so we can't mark them MOZ_STACK_CLASS. However, the tiny overhead that is saved by making these a raw pointer instead of RefPtr<Document> doesn't seem like it is worth the risk, so I think we should change these fields to a strong pointer.

Comment 1

[Andrew McCreight [:mccr8]]

Jan, could you take a look at this? Probably the biggest hassle here will be changing mDocument. to mDocument-> everywhere. Thanks.

Comment 2

[Jan Jaeschke [:jjaschke]]

The only way where this situation could occur is if these classes were used in new places in the code. TextDirectiveCreator has a protected constructor, its only public API is a static function, nothing can run script; therefore I think it's impossible for its Document reference to point to something deleted throughout its existence. Arguably, TextDirectiveFinder should get a private constructor + friend FragmentDirective to make sure it's not used outside of FragmentDirective (which keeps Document alive).

The motivation for this pattern was not performance, but instead documenting that Document is non-null through code (this way all users of mDocument wouldn't have to null-check).

However, this is a relatively weak argument. :) Therefore, patch attached.

Comment 3

[Jan Jaeschke [:jjaschke]]

Attachment: Bug 1974513 - Text Fragments: Use strong pointers to `Document` in helper classes. r=mccr8!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/6dc38deef8d0
https://hg.mozilla.org/integration/autoland/rev/a2c491995021
Text Fragments: Use strong pointers to `Document` in helper classes. r=mccr8

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c5055a75c84b
https://hg.mozilla.org/integration/autoland/rev/47ffe98b0a50
Revert "Bug 1974513 - Text Fragments: Use strong pointers to `Document` in helper classes. r=mccr8" for causing build bustages @TextDirectiveCreator.cpp.

Comment 6

[agoloman]

Backed out for causing build bustages @TextDirectiveCreator.cpp.

• Backout link
• Push with failures
• Failure Log

Comment 7

[Andrew McCreight [:mccr8]]

It looks like removing the redundant MOZ_ASSERT on nullness isn't optional!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/eda97e012f71
https://hg.mozilla.org/integration/autoland/rev/87a982a25f9d
Text Fragments: Use strong pointers to `Document` in helper classes. r=mccr8

Comment 9

[[e-mail]]

https://hg.mozilla.org/mozilla-central/rev/87a982a25f9d