1346623 - anonymous content created with nsIDocument::InsertAnonymousContent can change from non-native to native AC

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

We have the nsIDocument::InsertAnonymousContent API, exposed through nsIDOMWindowUtils, which allows script to create anonymous content and append it to the nsCanvasFrame.  This is used, among other things, by devtools to draw overlays in the page when selecting elements in the inspector.

Content added using this API is appended to a NAC div, nsCanvasFrame::mCustomContentContainer, but this content is not marked as NAC itself.  However, if the root element of the document is reframed, we end up cloning this custom content (in nsCanvasFrame::DestroyFrom), and returning it as part from nsCanvasFrame::CreateAnonymousContent, when we re-create the nsCanvasFrame.  The frame constructor calls SetNativeAnonymousBitOnDescendants on it, and this content is now considered native anonymous content.  This can change where we inherit styles from.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: chrome mochitest

Here's a chrome mochitest demonstrating the problem.  The anonymous content div should remain green, but due to reframing the nsCanvasFrame, it starts being considered as NAC, meaning we start inheriting styles from further up the tree.

Comment 2

[Cameron McCormack (:heycam)]

I haven't checked, so I don't know whether the anonymous content that devtools creates relies on normal inheritance in its AC subtree.  (It also creates ::before/::after pseudo-elements in the AC subtree, which is what was tripping up my changes in bug 1330843.)

We could note on the AC that it came from this API somehow, and so when recreating the nsCanvasFrame, we don't then mark it as native anonymous content.

Comment 3

[Cameron McCormack (:heycam)]

Testing with https://hg.mozilla.org/try/rev/468aa60c40c350af59b1c07a9162290176086f87 to force the nsIDocument::InsertAnonymousContent to be NODE_IS_NATIVE_ANONYMOUS from the start, it looks like at least the eye dropper tool isn't styled properly.  So I guess we need to ensure that we don't add the NAC bit in that reframing case.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: patch v0.1

How about something like this?

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8846399 [details] [diff] [review]
patch v0.1

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

This seems like the right approach. Please check in the mochitest too.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +4243,5 @@
> +    } else if (parentFrameType == nsGkAtoms::canvasFrame &&
> +               content == static_cast<nsCanvasFrame*>(aParentFrame)
> +                            ->GetCustomContentContainer()) {
> +      // don't mark descendants of the custom content container
> +      // as native anonymous

Please expand the comment to explain this in more detail, or to point to comment 0.

Also, worth re-ordering this to emphasize that we're still marking the root, just skipping the SetNativeAnonymousBitOnDescendants bit. I think we should probably keep the else {} block as-is, and make the SetNAtiveAnonymousBitOnDescendants call conditional.

@@ +4245,5 @@
> +                            ->GetCustomContentContainer()) {
> +      // don't mark descendants of the custom content container
> +      // as native anonymous
> +      content->SetIsNativeAnonymousRoot();
> +      content->SetFlags(NODE_IS_NATIVE_ANONYMOUS);

This line is redundant with the previous one.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8846393 [details]
chrome mochitest

Test looks good, though IIUC it needs a bit more work to be fully automated.

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Bug 1346623 - Part 1: Prevent canvas custom content from becoming NAC when reframing the root element.

Review commit: https://reviewboard.mozilla.org/r/121064/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121064/

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Bug 1346623 - Part 2: Add an API to get computed style values through an AnonymousContent object.

Review commit: https://reviewboard.mozilla.org/r/121066/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121066/

Comment 9

[Cameron McCormack (:heycam)]

Attachment: Bug 1346623 - Part 3: Test.

Review commit: https://reviewboard.mozilla.org/r/121068/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121068/

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8848089 [details]
Bug 1346623 - Part 1: Prevent canvas custom content from becoming NAC when reframing the root element.

https://reviewboard.mozilla.org/r/121064/#review123104

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8848090 [details]
Bug 1346623 - Part 2: Add an API to get computed style values through an AnonymousContent object.

https://reviewboard.mozilla.org/r/121066/#review123106

::: dom/webidl/AnonymousContent.webidl:86
(Diff revision 1)
> +  /**
> +   * Get the computed value of a property on an element inside this custom
> +   * anonymous content.
> +   */
> +  [Throws]
> +  DOMString? getComputedStylePropertyValue(DOMString elementId, 

Nit: Trailing whitespace.

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8848091 [details]
Bug 1346623 - Part 3: Test.

https://reviewboard.mozilla.org/r/121068/#review123112

Nice! Thanks for doing this.

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8848089 [details]
Bug 1346623 - Part 1: Prevent canvas custom content from becoming NAC when reframing the root element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121064/diff/1-2/

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8848090 [details]
Bug 1346623 - Part 2: Add an API to get computed style values through an AnonymousContent object.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121066/diff/1-2/

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8848091 [details]
Bug 1346623 - Part 3: Test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121068/diff/1-2/

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8848091 [details]
Bug 1346623 - Part 3: Test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121068/diff/2-3/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e40bc33e620
Part 1: Prevent canvas custom content from becoming NAC when reframing the root element. r=bholley
https://hg.mozilla.org/integration/autoland/rev/39ee9556d4a0
Part 2: Add an API to get computed style values through an AnonymousContent object. r=bholley
https://hg.mozilla.org/integration/autoland/rev/1f9a6ce59a56
Part 3: Test. r=bholley

Comment 18

[Carsten Book [:Tomcat]]

backed out for perma leak like https://treeherder.mozilla.org/logviewer.html#?job_id=84536073&repo=autoland

Comment 19

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0696366039f2
Backed out changeset 1f9a6ce59a56 
https://hg.mozilla.org/integration/autoland/rev/2f9cdacc5359
Backed out changeset 39ee9556d4a0 
https://hg.mozilla.org/integration/autoland/rev/6ed991361ec1
Backed out changeset 9e40bc33e620 for perma leak in chrome-style stylo tests

Comment 20

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ec155e82edb27d2b5fd7d63dba514bd41244030

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f85eb6b49794
Part 1: Prevent canvas custom content from becoming NAC when reframing the root element. r=bholley
https://hg.mozilla.org/integration/autoland/rev/f5a5a605ae82
Part 2: Add an API to get computed style values through an AnonymousContent object. r=bholley
https://hg.mozilla.org/integration/autoland/rev/cfe58f9ace35
Part 3: Test. r=bholley

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f85eb6b49794
https://hg.mozilla.org/mozilla-central/rev/f5a5a605ae82
https://hg.mozilla.org/mozilla-central/rev/cfe58f9ace35

Comment 23

[Matteo Ferretti [:zer0] [:matteo]]

We'd like to uplift bug 1349275, but it relies on the new `getComputedStylePropertyValue` method here added – by the way, thanks a lot for that, we really need it!

It would be possible uplift this bug too? It has some other dependencies…? Or it would be too risky…?

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

I would say too late for beta uplift. Maybe to aurora.

Comment 25

[Bobby Holley (:bholley)]

Not entirely sure what dependencies we'd need for the uplift (and how much we'd need to uplift to get what you want), so I'll leave this one to heycam.

Comment 26

[Cameron McCormack (:heycam)]

I am only moderately confident that uplifting part 1 is OK, but I'd have to dig into it to see what the implications are to be sure.  Since you're just looking for getComputedStylePropertyValue, which is in part 2, and a safer part to uplift, are you happy to uplift just that part?

Comment 27

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Cameron McCormack (:heycam) (away Apr 1-4) from comment #26)

> I am only moderately confident that uplifting part 1 is OK, but I'd have to
> dig into it to see what the implications are to be sure.  Since you're just
> looking for getComputedStylePropertyValue, which is in part 2, and a safer
> part to uplift, are you happy to uplift just that part?

I'm totally happy to uplift just that part, if the there are no others dependencies that prevent `getComputedStylePropertyValue` to works as expected. :)

Comment 28

[Cameron McCormack (:heycam)]

No other dependencies; uplifting just that part should be fine.

Comment 29

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8848090 [details]
Bug 1346623 - Part 2: Add an API to get computed style values through an AnonymousContent object.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1312103
[User impact if declined]: devtools' infobar won't be properly displayed anymore – after the scrolling, it would be misplaced outside the viewport.
[Is this code covered by automated tests?]: yes (the usage of the API introduced)
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: See Bug 1349275
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: It shouldn't
[Why is the change risky/not risky?]: It's a new API used mostly by devtools
[String changes made/needed]: None

This part is necessary for the proper fix of bug 1349275, that needs to be uplifted urgently in both aurora and beta, since the devtools' infobar would be broken otherwise. I talked with :pbro, that talked with :lizzard, and we should be able to get this on 53.

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Sorry, too late for 53. We still might be able to try this on 54, I leave it up to gchang.

Comment 31

[Gerry Chang [:gchang]]

Comment on attachment 8848090 [details]
Bug 1346623 - Part 2: Add an API to get computed style values through an AnonymousContent object.

Fix a display issue of devtools' infobar. Aurora54+.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6de2c310ebe8

Comment 33

[Emil Ghitta, QA [:emilghitta]]

I have verified that this issue is not reproducible anymore using Firefox 55.0a1 (Build Id:20170427030231) and Firefox 54.0b2 (Build Id:20170424145525) on Windows 10 64bit, Ubuntu 14.04 64bit and macOS 10.11.6.