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

RESOLVED FIXED in Firefox 26

Status

()

defect
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jruderman, Assigned: almasry.mina)

Tracking

(Blocks 1 bug, {assertion, regression, testcase})

Trunk
mozilla27
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox26 fixed, firefox27 fixed)

Details

(Whiteboard: [qa-])

Attachments

(3 attachments, 4 obsolete attachments)

Posted 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.
Posted 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?
Posted 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)
Posted 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-
Posted 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)
Posted 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)
Posted 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: 6 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.