Closed Bug 914029 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox26 --- fixed
firefox27 --- fixed

People

(Reporter: jruderman, Assigned: almasry.mina)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [qa-])

Attachments

(3 files, 4 obsolete files)

Attached file 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.
Attached file stack
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
Assignee: nobody → almasry.mina
Mina, do we update the validity of a fieldset when another fieldset is added as its descendant?
Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #816012 - Flags: review?(mounir)
Attached patch patch v2 (obsolete) — Splinter Review
Actually I think it might be a better idea to first call UpdateValidity and then update mInvalidElementsCount.
Attachment #816012 - Attachment is obsolete: true
Attachment #816012 - Flags: review?(mounir)
Attachment #817198 - Flags: review?(bugs)
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 on attachment 817198 [details] [diff] [review]
patch v2

I thought I had r- this.
Attachment #817198 - Flags: review?(bugs) → review-
Attached patch patch v3 (obsolete) — Splinter Review
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.
Attachment #817198 - Attachment is obsolete: true
Attachment #818043 - Flags: review?(bugs)
Attached patch patch v4 (obsolete) — Splinter Review
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
Attachment #818043 - Attachment is obsolete: true
Attachment #818043 - Flags: review?(bugs)
Attachment #818105 - Flags: review?(bugs)
Attachment #818105 - Flags: review?(bugs)
Attached patch patch v5Splinter Review
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
Attachment #818105 - Attachment is obsolete: true
Attachment #819038 - Flags: review?(bugs)
Attachment #819038 - Flags: review?(bugs) → review+
try push is green. marking checkin needed.
Keywords: checkin-needed
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
Attachment #819038 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/bf072b917172
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Attachment #819038 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.