Closed
Bug 598907
Opened 14 years ago
Closed 14 years ago
crash [@ _purecall | nsGenericHTMLFormElement::IsDisabled() ]
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla2.0b8
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
People
(Reporter: scoobidiver, Assigned: mounir)
References
(Blocks 2 open bugs)
Details
(Keywords: crash, regression)
Crash Data
Attachments
(1 file, 6 obsolete files)
5.89 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
Build : Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100922 Firefox/4.0b7pre This is a new crash signature that was introduced by b7pre/20100920 build. The crash daily rate is 2-6 crashes/day. It is #34 top crasher in b7pre/20100922 build. One comment says : "shutting down" Signature _purecall | nsGenericHTMLFormElement::IsDisabled() UUID 29d0fad7-fe59-4161-83cf-508262100922 Time 2010-09-22 10:59:00.11939 Uptime 17464 Last Crash 172007 seconds (2.0 days) before submission Install Age 17464 seconds (4.9 hours) since version was first installed. Product Firefox Version 4.0b7pre Build ID 20100922041040 Branch 2.0 OS Windows NT OS Version 6.1.7600 CPU x86 CPU Info GenuineIntel family 6 model 23 stepping 10 Crash Reason EXCEPTION_NONCONTINUABLE_EXCEPTION Crash Address 0x0 App Notes AdapterVendorID: 8086, AdapterDeviceID: 2a42 Crashing Thread Frame Module Signature [Expand] Source 0 mozcrt19.dll _purecall obj-firefox/memory/jemalloc/crtsrc/purevirt.c:54 1 xul.dll nsGenericHTMLFormElement::IsDisabled content/html/content/src/nsGenericHTMLElement.h:855 2 xul.dll nsGenericHTMLFormElement::IsDisabled content/html/content/src/nsGenericHTMLElement.h:855 3 xul.dll nsHTMLTextAreaElement::UpdateBarredFromConstraintValidation content/html/content/src/nsHTMLTextAreaElement.cpp:1223 4 xul.dll nsHTMLTextAreaElement::UnbindFromTree content/html/content/src/nsHTMLTextAreaElement.cpp:1034 5 xul.dll nsGenericElement::UnbindFromTree content/base/src/nsGenericElement.cpp:3046 6 xul.dll nsStyledElement::UnbindFromTree content/base/src/nsStyledElement.cpp:244 7 xul.dll nsGenericHTMLElement::UnbindFromTree content/html/content/src/nsGenericHTMLElement.cpp:969 8 xul.dll nsGenericElement::UnbindFromTree content/base/src/nsGenericElement.cpp:3046 9 xul.dll nsStyledElement::UnbindFromTree content/base/src/nsStyledElement.cpp:244 10 xul.dll nsGenericHTMLElement::UnbindFromTree content/html/content/src/nsGenericHTMLElement.cpp:969 11 xul.dll nsGenericHTMLFormElement::UnbindFromTree content/html/content/src/nsGenericHTMLElement.cpp:2534 12 xul.dll nsGenericElement::cycleCollection::Unlink content/base/src/nsGenericElement.cpp:4338 13 xul.dll nsEventListenerManager::cycleCollection::Unlink content/events/src/nsEventListenerManager.cpp:348 14 xul.dll xul.dll@0xd9c2b7 15 xul.dll nsCycleCollector::FinishCollection xpcom/base/nsCycleCollector.cpp:2695 16 xul.dll nsCycleCollector::Collect xpcom/base/nsCycleCollector.cpp:2490 17 @0x6850cf 18 xul.dll PL_DHashTableOperate obj-firefox/xpcom/build/pldhash.c:625 19 @0x30e0e193 The regression range is : http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7640eb022be6&tochange=b1d56e69f4d9 More reports at : http://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A4.0b7pre&query_search=signature&query_type=exact&query=_purecall%20|%20nsGenericHTMLFormElement%3A%3AIsDisabled%28%29&date=09%2F23%2F2010%2004%3A59%3A51&range_value=4&range_unit=weeks&hang_type=any&process_type=any&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&admin=&signature=_purecall%20|%20nsGenericHTMLFormElement%3A%3AIsDisabled%28%29
Assignee | ||
Comment 2•14 years ago
|
||
When the element is unbind from tree, mFieldSet is updated depending of the new parent chain. If an entire sub-tree is removed, mFieldSet will still have a value and UnbindFromTree might not be called again. In that case, the CC should remove the subtree at some point but can it happen to have the fieldset removed and not the elements inside?
Component: XPCOM → DOM: Core & HTML
Keywords: testcase-wanted
OS: Windows 7 → All
QA Contact: xpcom → general
Hardware: x86 → All
Comment 3•14 years ago
|
||
> If an entire sub-tree is removed, mFieldSet will still have a > value and UnbindFromTree might not be called again. How so? You mean if the subtree includes the fieldset? And it won't be called again because in the destructor we only do shallow unbinds? > can it happen to have the fieldset removed and not the elements inside? I think that can happen, since we don't hold strong refs to parent nodes... smaug had a possible patch for that, though.
Comment 4•14 years ago
|
||
Yeah, but no idea whether anything for that will land for ff4. And note, during teardown child nodes will release parent pointers...
Comment 5•14 years ago
|
||
Thats fine. I bet what happens here is that during CC #1 the fieldset goes away but NOT its kids (which get a shallow unbind but don't unbind their descendants). Then in a later CC the descendants are CCed and we get the stack above, since the fieldset is long-dead.
Assignee: nobody → mounir.lamouri
blocking2.0: ? → final+
In CC-unlink we intentionally do a shallow UnbindFromTree to save cycles, similar to how we do the same thing in the element dtor. An exception is documents whose unlink function which does a deep UnbindFromTree in order to clear the is-in-doc bit of all nodes.
Comment 7•14 years ago
|
||
We don't do shallow unbind in CC. We do shallow unbind in dtor.
Ugh, you're right, we should change that
Comment 9•14 years ago
|
||
Yeah, the point is that the shallow-unbind optimization is broken in cases when the kid is caching weak pointers to ancestors... Perhaps mFieldSet should just be a strong pointer? The other option is for ~nsHTMLFieldSetElement to clear the mFieldSet, just like ~nsHTMLFormElement drops mForm.
> The other option is for ~nsHTMLFieldSetElement to clear
> the mFieldSet, just like ~nsHTMLFormElement drops mForm.
That is an excellent idea!
Note that during CC, none of the elements should be deleted until they are all unlinked, so no pointers should be dangling. Of course, if elements accesses pointers in the dtor you are out of luck.
Comment 11•14 years ago
|
||
If we _do_ want to do that, then we need to do it not only in ~nsHTMLFieldSetElement but also in the fieldset's Unlink, because it drops references to its form controls in Unlink (and if its Unlink is being called, it's about to go away anyway).
Though as long as unlink uses deep UnbindFromTree, form controls should be clearing their mFieldSet themselves, no?
Comment 13•14 years ago
|
||
Oh, indeed. It's sort of unfortunate that we do a deep unbind there, but does mean that the dtor is the only place we need to worry about.
Reporter | ||
Comment 14•14 years ago
|
||
See bug 561277 for crash about UnbindFromTree
Assignee | ||
Comment 15•14 years ago
|
||
We can't easily inform all fieldset descendants when the fieldset is destroyed because the fieldsets might not now its descendants (IOW, mElements might be null). So, to have this working we should eager initialize this but that might create some troubles because no events are sent on parsing and mElements would be wrong. One solution might be to create mElements in DomeAddingChildren or DoneCreateElement maybe? Do someone see another solution?
Assignee | ||
Comment 16•14 years ago
|
||
(In reply to comment #4) > Yeah, but no idea whether anything for that will land for ff4. What's is the bug number?
Assignee | ||
Comment 17•14 years ago
|
||
Attachment #479685 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 18•14 years ago
|
||
Comment on attachment 479685 [details] [diff] [review] Patch #2 v1 Henri, can you review the parser/content sink changes? ... 2 lines :) Olli, could you review the rest?
Attachment #479685 -
Flags: review?(hsivonen)
Assignee | ||
Updated•14 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 19•14 years ago
|
||
By the way, this patch is purely theoretical. I wasn't able to reproduce the same situation so I don't know if that's really fixing it...
Comment 20•14 years ago
|
||
Comment on attachment 479685 [details] [diff] [review] Patch #2 v1 http://mxr.mozilla.org/mozilla-central/source/content/xml/document/src/nsXMLContentSink.cpp#573 also needs to be updated. r=me on the parser stuff if you update the XML side, too.
Attachment #479685 -
Flags: review?(hsivonen) → review+
Comment 21•14 years ago
|
||
Comment on attachment 479685 [details] [diff] [review] Patch #2 v1 > nsHTMLFieldSetElement::~nsHTMLFieldSetElement() > { >+ NS_ASSERTION(mElements, "mElements shouldn't be null!"); >+ >+ // TODO: should be removed with bug 600773. >+ if (mElements) { >+ PRUint32 length = mElements->Length(PR_TRUE); Hmm, is this really safe? Is it ok to flush parser during CC or while? I'd pass PR_FALSE, especially because I don't understand why you need to flush. (Although, the element shouldn't be in the document, so no flushing should actually happen)
Attachment #479685 -
Flags: review?(Olli.Pettay) → review+
Comment 22•14 years ago
|
||
> because the fieldsets might not now its descendants (IOW, mElements might be
> null).
Why not just have the descendants register and unregister with their mFieldset when it changes? Again, that's how we do forms.
Assignee | ||
Comment 23•14 years ago
|
||
(In reply to comment #22) > > because the fieldsets might not now its descendants (IOW, mElements might be > > null). > > Why not just have the descendants register and unregister with their mFieldset > when it changes? Again, that's how we do forms. I was going to say that this would cost more because it would require to look at the entire parent chain but we already do that :-/. The only con would be that this will require us to create a nsCOMArray to keep track of form control elements that are actually depending of the fieldset. BTW, if we do that, I guess we could remove nsIMutationObserver from nsHTMLFieldSetElement and have something similar to nsHTMLFormElement for .elements.
Well, we'd likely have to walk the parent chain again, so I'd still stay it costs us. I would prefer not to do that given that fieldsets are very rare. So IMHO it's better to spend a few more cycles when they are used, and instead optimize the case when they aren't.
Assignee | ||
Comment 25•14 years ago
|
||
(In reply to comment #24) > Well, we'd likely have to walk the parent chain again, so I'd still stay it > costs us. Why again? We are currently walking the parent chain looking for a fieldset (which make us walk the entire parent chain in most case). We should be able to use this walk to set everything we need?
Comment 26•14 years ago
|
||
I have this crash regular. If it helps, most of the time it is after I pressed CTRL+R, sometimes also when it is closing firefox to apply the update. But not sure it is really causing it.
Assignee | ||
Comment 27•14 years ago
|
||
This patch should be better.
Attachment #479685 -
Attachment is obsolete: true
Attachment #484703 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 28•14 years ago
|
||
Attachment #484703 -
Attachment is obsolete: true
Attachment #484709 -
Flags: review?(Olli.Pettay)
Attachment #484703 -
Flags: review?(Olli.Pettay)
Comment 29•14 years ago
|
||
Why a COMArray? It seems like using an nsTArray<nsRefPtr<nsGenericFormElement>> would prevent a lot of casting.
Assignee | ||
Comment 30•14 years ago
|
||
Exact, nsTArray<nsGenericHTMLFormElement*> should even be enough.
Attachment #484709 -
Attachment is obsolete: true
Attachment #485276 -
Flags: review?(Olli.Pettay)
Attachment #484709 -
Flags: review?(Olli.Pettay)
Comment 31•14 years ago
|
||
Or maybe a nsTPtrArray<nsGenericHTMLFormElement> ?
Assignee | ||
Comment 32•14 years ago
|
||
I didn't know nsTPtrArray. Thanks,
Attachment #485276 -
Attachment is obsolete: true
Attachment #485297 -
Flags: review?(Olli.Pettay)
Attachment #485276 -
Flags: review?(Olli.Pettay)
Comment 33•14 years ago
|
||
Comment on attachment 485297 [details] [diff] [review] Patch v3.2 >+ // List of elements which have this fieldset as first fieldset ancestor. >+ nsTPtrArray<nsGenericHTMLFormElement> mDependantElements; >+ Nit: I think you mean "dependent". I wouldn't submit a new patch for that just yet, though.
Comment 34•14 years ago
|
||
Comment on attachment 485297 [details] [diff] [review] Patch v3.2 I don't understand how the unbind case works. Sure, mFieldSet won't point to deleted object, but how does mFieldSet get cleared? Re-ask reviewed if I missed something.
Attachment #485297 -
Flags: review?(Olli.Pettay) → review-
Assignee | ||
Comment 35•14 years ago
|
||
This should be better. Inter-diff can be found at: http://mounir.pastebin.mozilla.org/829050 (+ s/mDependantElements/mDependentElemest/g)
Attachment #485297 -
Attachment is obsolete: true
Attachment #486579 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 36•14 years ago
|
||
Calling RemoveElement() on dtor.
Attachment #486579 -
Attachment is obsolete: true
Attachment #486586 -
Flags: review?(Olli.Pettay)
Attachment #486579 -
Flags: review?(Olli.Pettay)
Updated•14 years ago
|
Attachment #486586 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs landing]
Assignee | ||
Comment 37•14 years ago
|
||
Pushed: http://hg.mozilla.org/mozilla-central/rev/132b620273ac
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b8
Reporter | ||
Comment 38•14 years ago
|
||
This bug is supposed to be fixed in 4.0b8pre/20101106 build, but crashes with this signature happen after. See: http://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A4.0b8pre&query_search=signature&query_type=exact&query=_purecall%20|%20nsGenericHTMLFormElement%3A%3AIsDisabled%28%29&range_value=4&range_unit=weeks&hang_type=any&process_type=any&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&admin=&signature=_purecall%20|%20nsGenericHTMLFormElement%3A%3AIsDisabled%28%29 So bug 612094 is probably a dupe of this bug, even if 4.0b7 does not include the changeset in comment 37.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 40•14 years ago
|
||
I see only 4 crashes that _might_ have happen with this fix: http://crash-stats.mozilla.com/report/index/f2eed5f4-e96a-40b9-a2aa-85fdf2101107 http://crash-stats.mozilla.com/report/index/90ba1251-3cbd-46d4-8327-245542101109 http://crash-stats.mozilla.com/report/index/036c1c8c-97a3-4077-aeba-b240f2101109 http://crash-stats.mozilla.com/report/index/07754a64-09c9-4930-92be-29b7e2101109 (the three last ones are obviously the same machine) All other crashes after the fix happened with an outdated version. It looks like, the crashes happened with these changesets: 56664 and 56683. And the fix came with 56935 Based on that, I'm going to mark this RESOLVED FIXED again. Feel free to reopen if that feels wrong.
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Crash Signature: [@ _purecall | nsGenericHTMLFormElement::IsDisabled() ]
Updated•9 years ago
|
Keywords: testcase-wanted
You need to log in
before you can comment on or make changes to this bug.
Description
•