stylo: Make nsTextControlFrame::AppendAnonymousContentTo safe to call from parallel style traversal

RESOLVED FIXED in Firefox 51

Status

()

Core
Editor
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Bobby Holley (parental leave - send mail for anything urgent), Assigned: Bobby Holley (parental leave - send mail for anything urgent))

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(3 attachments)

In Stylo, we need to be able to find native anonymous content during parallel traversal. This involves doing a QueryFrame to nsIAnonymousContentCreator and then calling AppendAnonymousContentTo. This generally works fine, because most NAC-creating frames just cache the anonymous content as a member. But nsTextFrame does various not-thread-safe things, which is what this bug is designed to fix.

In bug 1294915, we are exploring static analysis approaches to detecting these issues a la rooting analysis.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2440c3f2e2b2
Created attachment 8781331 [details] [diff] [review]
Part 1 - Add missing include. v1

Including this header without this causes compilation problems.
Attachment #8781331 - Flags: review?(ehsan)
Created attachment 8781332 [details] [diff] [review]
Part 2 - Avoid QIing a DOM node in nsTextControlFrame::AppendAnonymousContentTo. v1

This is problematic when we want to call AppendAnonymousContentTo off-main-thread.
Created attachment 8781333 [details] [diff] [review]
Part 3 - Stop lazily creating the root node in nsTextEditorState::GetRootFrame.

AFAICT this doesn't change behavior, since all the callers of GetRootNode should
occur after we've already called BindToFrame. However, it makes it easier for
future static analysis to see that we don't trigger node creation from
nsTextControlFrame::AppendAnonymousContentTo.
Attachment #8781333 - Flags: review?(ehsan)
Attachment #8781332 - Flags: review?(ehsan)

Updated

a year ago
Attachment #8781331 - Flags: review?(ehsan) → review+

Updated

a year ago
Attachment #8781332 - Flags: review?(ehsan) → review+

Comment 5

a year ago
Comment on attachment 8781333 [details] [diff] [review]
Part 3 - Stop lazily creating the root node in nsTextEditorState::GetRootFrame.

Review of attachment 8781333 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/nsTextEditorState.cpp
@@ +1156,5 @@
>    }
>  
>    mBoundFrame = aFrame;
>  
> +  CreateRootNode();

Can you please check the return value as well?
Attachment #8781333 - Flags: review?(ehsan) → review+

Comment 6

a year ago
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a07c43ca2cc
Add missing include. r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ba56b086fe3
Avoid QIing a DOM node in nsTextControlFrame::AppendAnonymousContentTo. r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/a60d54195a3a
Stop lazily creating the root node in nsTextEditorState::GetRootFrame. r=ehsan

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6a07c43ca2cc
https://hg.mozilla.org/mozilla-central/rev/1ba56b086fe3
https://hg.mozilla.org/mozilla-central/rev/a60d54195a3a
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.