914029 - "Assertion failure: mInvalidElementsCount >= 0" with multiple <fieldset> elements

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

1. Load the testcase (in a debug build)
2. Force a GC (or quit the browser)

Assertion failure: mInvalidElementsCount >= 0, at content/html/content/src/HTMLFieldSetElement.cpp:271

This code was added in bug 717181.

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Mina Almasry]

Uh oh... Sorry about this...

So in the patch for bug 717181, we have tests for nested fieldsets, but not one that is added dynamically. Is that why? I'm also not sure why HTMLFieldSetElement::RemoveElement is being called from this test case at all. It seems to me that all that'd be happening here is HTMLFieldSetElement::AddElement

Comment 3

[Mounir Lamouri (:mounir)]

Mina, do we update the validity of a fieldset when another fieldset is added as its descendant?

Comment 4

[Mina Almasry]

Attachment: patch v1

Right, the problem was that we were not adding mInvalidElementsCount of nested fieldsets to their parents when we were adding a fieldset as a child of another, and vice versa when removing a fieldset from a fieldset parent.

Here is a patch. Please let me know if you are not reviewing patches anymore.

Comment 5

[Mina Almasry]

Attachment: patch v2

Actually I think it might be a better idea to first call UpdateValidity and then update mInvalidElementsCount.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 817198 [details] [diff] [review]
patch v2

Could you not use QO, but FromContent.
Something like 
HTMLFieldSetElement* fieldSet = HTMLFieldSetElement::FromContent(aElement);

Why mInvalidElementsCount - 1 ?

We need some tests here, and assertions too.
Could you perhaps add some debug code to 
Add/RemoveElement which go through all the relevant elements and checks their
validity state and counts and then compares to mInvalidElementsCount

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 817198 [details] [diff] [review]
patch v2

I thought I had r- this.

Comment 8

[Mina Almasry]

Attachment: patch v3

So changes made and test added.

The reason it's mInvalidElementsCount - 1 is that the call to UpdateValidity increments/decrements the count by 1, and what we have to do here is augment the count to account for _all_ the invalid elements in the fieldset that we are adding, and not just 1.

Comment 9

[Mina Almasry]

Attachment: patch v4

Actually, I pushed the last patch to try and I messed up: I should check that cvElmt is not null before derefrencing it. Here is an updated patch.

Try: https://tbpl.mozilla.org/?tree=Try&rev=d0f048ba8f08

Comment 10

[Mina Almasry]

Attachment: patch v5

Okay I ran the tests that failed in the last try push locally and they pass so I feel good about this. 

Try push: https://tbpl.mozilla.org/?tree=Try&rev=e3a4ebd3f9f5

Comment 11

[Mina Almasry]

try push is green. marking checkin needed.

Comment 12

[Mina Almasry]

Comment on attachment 819038 [details] [diff] [review]
patch v5

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 717181
User impact if declined: MOZ_ASSERT fails. Broken fieldset psuedo classes.
Testing completed (on m-c, etc.):
Try push: https://tbpl.mozilla.org/?tree=Try&rev=e3a4ebd3f9f5
Risk to taking this patch (and alternatives if risky): Minimal. Only fixed the parts that caused the MOZ_ASSERT to fail.
String or IDL/UUID changes made by this patch: None

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf072b917172

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bf072b917172

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a466c52b4805