1290904 - Assertion failure: debugInvalidElementsCount == mInvalidElementsCount, at HTMLFieldSetElement.cpp:309

Status: RESOLVED FIXED

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

Description

[Carsten Book [:Tomcat]]

Attachment: complete stack

Found via bughunter and reproduces with the latest tinderbox m-c debug windows build.

Seems to be nightly only so far.

Steps to reproduce :
-> Load https://www.hainanairlines.com/US/US/Home
---> Assertion failure 

Assertion failure: debugInvalidElementsCount == mInvalidElementsCount, at c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/dom/html/HTMLFieldSetElement.cpp:309

Assertion failure: debugInvalidElementsCount == mInvalidElementsCount, at c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/dom

#01: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x188e510]
#02: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x188df35]
#03: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x1847d50]
#04: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0xe6eb87]
#05: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x1847f96]
#06: NS_DebugBreak[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x1a887c]
#07: NS_DebugBreak[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x1a8285]
#08: NS_DebugBreak[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x1b0cbb]
#09: NS_DebugBreak[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x1b40a1]
#10: NS_StringContainerFinish[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x22b737]
#11: XRE_TermEmbedding[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x25e801d]
#12: ???[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\firefox.exe +0x182f]
#13: ???[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\firefox.exe +0x15b2]
#14: ???[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\firefox.exe +0x201e]
#15: TargetNtUnmapViewOfSection[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\firefox.exe +0x319e5]

Comment 1

[Boris Zbarsky [:bzbarsky]]

I can't reproduce in a Mac debug build from revision 4a18b5cacb1b.

Comment 2

[Carsten Book [:Tomcat]]

yeah seems windows only

Comment 3

[Carsten Book [:Tomcat]]

Attachment: test-crash-on-load.js

Comment 4

[Carsten Book [:Tomcat]]

maybe this requires spider to reproduce

firefox -spider -start -quit -hook http://path-to-test-crash-on-load.js -url https://www.hainanairlines.com/US/US/Home

You may need to repeat this several times to get it to reproduce.

Get Spider from http://bclary.com/projects/spider/spider-0.1.0.3-sm+tb+fx+an+fn.xpi and the user hook is attached.

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK, I reproduced this with a useful stack (unlike the stack in comment 0, which has no bearing on reality).  The real stack is:

0  mozilla::dom::HTMLFieldSetElement::RemoveElement (this=0x139b88030, aElement=0x138d66a30) at HTMLFieldSetElement.cpp:309
#1  0x000000010422bf5d in nsGenericHTMLFormElement::UpdateFieldSet (this=0x138d66a30, aNotify=false) at nsGenericHTMLElement.cpp:2557
#2  0x000000010422c0fc in nsGenericHTMLFormElement::UnbindFromTree (this=0x138d66a30, aDeep=true, aNullParent=true) at nsGenericHTMLElement.cpp:2090
#3  0x000000010415b38a in mozilla::dom::HTMLInputElement::UnbindFromTree (this=0x138d66a30, aDeep=true, aNullParent=true) at HTMLInputElement.cpp:4793
#4  0x000000010295e4cc in mozilla::dom::FragmentOrElement::cycleCollection::Unlink (this=0x10b228730, p=0x139b9ac00) at FragmentOrElement.cpp:1385
#5  0x000000010412962d in mozilla::dom::HTMLFormElement::cycleCollection::Unlink (this=0x10b228730, p=0x139b9ac00) at HTMLFormElement.cpp:145
#6  0x0000000100550b09 in nsCycleCollector::CollectWhite (this=0x11c222000) at nsCycleCollector.cpp:3343

debugInvalidElementsCount is 0 and mInvalidElementsCount is 1.

I guess I'll try to catch this in rr when I get back to a place where I can run rr.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Get Spider from http://bclary.com/projects/spider/spider-0.1.0.3-sm+tb+fx+an+fn.xpi

Firefox refuses to install from there...  Do you recall how I can twiddle that setting?

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK, I got the extension installed and have now tried to reproduce 30 or 40 times (on Linux).  I have yet to reproduce it.  :(

I'm going to stop spending time on this now...

Comment 8

[Bob Clary [:bc] (inactive)]

I reproduced on Fedora 23, dual x86_64 with a build from this morning (not a clobber though) twice by manually starting from the command line then shutting down by clicking the (x) and once via Spider. Occurs during shutdown. Tested with an existing profile and a new default profile.

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Edgar, would you be interested in reproducing this and trying to catch this using rr?

Comment 10

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #9)
> Hi Edgar, would you be interested in reproducing this and trying to catch
> this using rr?

I will try to reproduce the crash. Keep ni to me for tracking.

Comment 11

[Edgar Chen [:edgar]]

I could reproduce on Linux build from revision 3e73fd638e68. And I got same stack as comment #5.

Step:
- Install spider from http://bclary.com/projects/spider/spider-0.1.0.3-sm+tb+fx+an+fn.xpi
- Run "./mach run -spider -start -quit -hook https://bug1290904.bmoattachments.org/attachment.cgi\?id\=8776855 -url https://www.hainanairlines.com/US/US/Home"

I am going to try to catch this in rr.

Comment 12

[Edgar Chen [:edgar]]

The problem is that we are not decreasing mInvalidElementsCount of nested fieldsets to their parents correctly when we are removing a fieldset from a fieldset parent [1]. The removing fieldset's mInvalidElementsCount could be more than 1, but we only call parent's UpdateValidity() once. 
Vice versa when adding a fieldset as child of another fieldset.

[1] https://dxr.mozilla.org/mozilla-central/rev/3b80868f7a8fe0361918a814fbbbfb9308ae0c0a/dom/html/HTMLFieldSetElement.cpp#274-284

Comment 13

[Edgar Chen [:edgar]]

Attachment: testcase

testcase to reproduce this assertion.

Comment 14

[Edgar Chen [:edgar]]

Attachment: Bug 1290904 - Fix assertion failure on removing/adding a fieldsets on a nested fieldset;

Review commit: https://reviewboard.mozilla.org/r/90598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90598/

Comment 15

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

Would other option be to just change the #DEBUG code doing the assertion?
>    HTMLFieldSetElement* fieldSet = FromContent(mDependentElements[i]);
>    if (fieldSet) {
>      debugInvalidElementsCount += fieldSet->mInvalidElementsCount;
>      continue;

That could just increase debugInvalidElementsCount by one, not by fieldSet->mInvalidElementsCount?

Comment 16

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

I mean, if fieldset has non-zero mInvalidElementsCount, then increase debugInvalidElementsCount by one.

Comment 17

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

Comment on attachment 8807458 [details]
Bug 1290904 - Fix assertion failure on removing/adding a fieldsets on a nested fieldset;

https://reviewboard.mozilla.org/r/90598/#review90346

This makes nested fieldsets tiny bit slower, but code easier to understand so fine.

Comment 18

[Edgar Chen [:edgar]]

(In reply to Olli Pettay [:smaug] from comment #15)
> Would other option be to just change the #DEBUG code doing the assertion?
> >    HTMLFieldSetElement* fieldSet = FromContent(mDependentElements[i]);
> >    if (fieldSet) {
> >      debugInvalidElementsCount += fieldSet->mInvalidElementsCount;
> >      continue;
> 
> That could just increase debugInvalidElementsCount by one, not by
> fieldSet->mInvalidElementsCount?

(In reply to Olli Pettay [:smaug] from comment #16)
> I mean, if fieldset has non-zero mInvalidElementsCount, then increase
> debugInvalidElementsCount by one.

Consider following case,

  <fieldset id="outer">
    <fieldset>
      <fieldset id="inner">
      </fieldset>
    </fieldset>
  </fieldset>
  <script>
  function createInvaildTextAreaElement() {
    var textarea = document.createElement("textarea");
    textarea.setAttribute("required", "");
    return textarea;
  }

  var innerFieldset = document.getElementById('inner');
  var outerFieldset = document.getElementById('outer');

  innerFieldset.appendChild(createInvaildTextAreaElement());
  innerFieldset.appendChild(createInvaildTextAreaElement());
  // outerFieldset's mInvalidElementsCount had been increased to 2.

  outerFieldset.appendChild(createInvaildTextAreaElement());
  // outerFieldset's mInvalidElementsCount now is 3.
  // Assertion when checking outerFieldset's mInvalidElementsCount (debugInvalidElementsCount is 2).
  </script>

Unfortunately, changing #DEBUG code doesn't totally fix such counter synchronization problems, unless we also change the whole logic of updating parent's mInvalidElementsCount. :(

Comment 19

[Edgar Chen [:edgar]]

Comment on attachment 8807458 [details]
Bug 1290904 - Fix assertion failure on removing/adding a fieldsets on a nested fieldset;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90598/diff/1-2/

Comment 20

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

ok

Comment 21

[Edgar Chen [:edgar]]

Comment on attachment 8807458 [details]
Bug 1290904 - Fix assertion failure on removing/adding a fieldsets on a nested fieldset;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90598/diff/2-3/

Comment 22

[Edgar Chen [:edgar]]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e1b6866877d16c2c484b513c5eba49ab110eba1&filter-tier=1&group_state=expanded

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b014c76108fa
Fix assertion failure on removing/adding a fieldsets on a nested fieldset; r=smaug

Comment 24

[Phil Ringnalda (:philor)]

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

Comment 25

[Carsten Book [:Tomcat]]

edgar, bughunter shows this affects beta builds too so we need to uplift or ?

Comment 26

[Edgar Chen [:edgar]]

Comment on attachment 8807458 [details]
Bug 1290904 - Fix assertion failure on removing/adding a fieldsets on a nested fieldset;

Approval Request Comment
[Feature/regressing bug #]:
A bug exists for a long time.

[User impact if declined]:
browser/tab crashes in debug build on specific steps
(Add/Remove a fielset with > 1 invalid elements count into/from a nested fieldset).

[Describe test coverage new/current, TreeHerder]:
A new test added verifying this case.

[Risks and why]:
Low. The patch is easily to understand. And it has been in m-c for a while and doesn't cause any regression.

[String/UUID change made/needed]:
None.

Comment 27

[Gerry Chang [:gchang]]

Comment on attachment 8807458 [details]
Bug 1290904 - Fix assertion failure on removing/adding a fieldsets on a nested fieldset;

Fix an assertion. Beta51+. Should be in 51 beta 2.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c8e0d652196f