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.
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?
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.
Actually I think it might be a better idea to first call UpdateValidity and then update mInvalidElementsCount.
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-
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.
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
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 #819038 - Flags: review?(bugs) → review+
try push is green. marking 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?
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Attachment #819038 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.