898926 - "Assertion failure: mElements.Contains(cur)" with xbl:children, adding stylesheet

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: mElements.Contains(cur), at layout/style/nsCSSRuleProcessor.cpp:3505

More fallout from bug 653881?

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Andrew McCreight [:mccr8]]

That doesn't sound great, but feel free to adjust the rating as desired.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

I'm not convinced that this is a security sensitive bug, but I don't know all of the possible consequences, so I'll leave it as is.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: xbl:children elements are insertion points if they're inactive.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Add reftest.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Add reftest.

Sorry for the spam -- I got overeager in simplifying the reftest and the
previous version didn't assert, even before the patch. This reftest
does, though.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 786613 [details] [diff] [review]
Add reftest.

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

::: layout/reftests/dom/xbl-children-4.xhtml
@@ +13,5 @@
> +    <script>
> +        onload = function() {
> +            var newSheet = document.createElementNS("http://www.w3.org/1999/xhtml", "style");
> +            newSheet.appendChild(document.createTextNode("#nosuchelement { }"));
> +            document.head.appendChild(newSheet);

Please see if you can change this to just do layout flush by accessing document.body.offsetTop or some such.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec3734c7c17f
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f28258bc04f

I added some comments to the reftest to make it more clear as to what was going on.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec3734c7c17f
https://hg.mozilla.org/mozilla-central/rev/7f28258bc04f

Comment 10

[Al Billings [:abillings - ex-MoCo]]

This was fixed in 26 but no one ever said how far back it went (and it didn't go through sec-approval, which implies trunk only at the time).

Are earlier versions unaffected? I'm specifically concerned about a potential unfixed sec-high in ESR24 or ESR17.

Comment 11

[Daniel Veditz [:dveditz]]

Don't we want to uplift this to Firefox 25?

Comment 12

[Alex Keybl [:akeybl]]

Just needs an approval request

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 786583 [details] [diff] [review]
xbl:children elements are insertion points if they're inactive.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 653881
User impact if declined: Potential security hole.
Testing completed (on m-c, etc.): This has been on m-c for a while, as well as aurora

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Al Billings [:abillings] from comment #10)
> This was fixed in 26 but no one ever said how far back it went (and it
> didn't go through sec-approval, which implies trunk only at the time).

Yeah, this doesn't affect anything other than Beta at this point.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 786583 [details] [diff] [review]
xbl:children elements are insertion points if they're inactive.

Let's not ship this. :-)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bec39903e303
https://hg.mozilla.org/releases/mozilla-beta/rev/5f1bc0dcd086