Closed Bug 1400701 Opened 3 years ago Closed 3 years ago

Assertion failure: IsIdle(oldState) in [@ PLDHashTable::Remove]

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: tsmith, Assigned: ehsan)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(2 files)

Attached file test_case.html
Assertion failure: IsIdle(oldState), at /src/xpcom/ds/PLDHashTable.h:132

#0 0x7fd76edf3541 in Checker::StartWriteOp() /src/xpcom/ds/PLDHashTable.h:130:5
#1 0x7fd76eddbb86 in PLDHashTable::Remove(void const*) /src/xpcom/ds/PLDHashTable.cpp:624:15
#2 0x7fd7711f4219 in mozilla::nsTextNodeDirectionalityMap::RemoveEntryForProperty(mozilla::dom::Element*) /src/dom/base/DirectionalityUtils.cpp:491:17
#3 0x7fd7711f40e4 in mozilla::nsTextNodeDirectionalityMap::nsTextNodeDirectionalityMapPropertyDestructor(void*, nsIAtom*, void*, void*) /src/dom/base/DirectionalityUtils.cpp:462:12
#4 0x7fd7714678c5 in nsPropertyTable::PropertyList::DeletePropertyFor(nsPropertyOwner) /src/dom/base/nsPropertyTable.cpp:296:5
#5 0x7fd771468433 in nsPropertyTable::DeleteProperty(nsPropertyOwner, nsIAtom*) /src/dom/base/nsPropertyTable.cpp:232:23
#6 0x7fd77140d262 in nsINode::DeleteProperty(unsigned short, nsIAtom*) /src/dom/base/nsINode.cpp:195:41
#7 0x7fd7711f3ce8 in mozilla::nsTextNodeDirectionalityMap::ClearEntry(nsPtrHashKey<mozilla::dom::Element>*, void*) /src/dom/base/DirectionalityUtils.cpp:565:15
#8 0x7fd7711f3bd5 in nsCheapSet<nsPtrHashKey<mozilla::dom::Element> >::EnumerateEntries(nsCheapSetOperator (*)(nsPtrHashKey<mozilla::dom::Element>*, void*), void*) /src/xpcom/ds/nsCheapSets.h:88:15
#9 0x7fd7711f383f in mozilla::nsTextNodeDirectionalityMap::EnsureMapIsClear(nsINode*) /src/dom/base/DirectionalityUtils.cpp:585:17
#10 0x7fd771498ce7 in nsTextNode::UnbindFromTree(bool, bool) /src/dom/base/nsTextNode.cpp:155:3
#11 0x7fd7711bf9e4 in mozilla::dom::Element::UnbindFromTree(bool, bool) /src/dom/base/Element.cpp:1992:37
#12 0x7fd773131bca in nsGenericHTMLElement::UnbindFromTree(bool, bool) /src/dom/html/nsGenericHTMLElement.cpp:537:20
#13 0x7fd771414ea8 in nsINode::doRemoveChildAt(unsigned int, bool, nsIContent*, nsAttrAndChildArray&) /src/dom/base/nsINode.cpp:1945:9
#14 0x7fd7711e5e15 in mozilla::dom::FragmentOrElement::RemoveChildAt(unsigned int, bool) /src/dom/base/FragmentOrElement.cpp:1369:5
#15 0x7fd7714162f7 in nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) /src/dom/base/nsINode.cpp:2432:5
#16 0x7fd771991be0 in mozilla::dom::NodeBinding::replaceChild(JSContext*, JS::Handle<JSObject*>, nsINode*, JSJitMethodCallArgs const&) /src/obj-firefox/dom/bindings/NodeBinding.cpp:955:45
#17 0x7fd772b3e96e in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /src/dom/bindings/BindingUtils.cpp:3055:13
#18 0x7fd777b0e881 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /src/js/src/jscntxtinlines.h:293:15
#19 0x7fd777b0e42d in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:495:16
#20 0x7fd777b0f2c5 in InternalCall(JSContext*, js::AnyInvokeArgs const&) /src/js/src/vm/Interpreter.cpp:540:12
#21 0x7fd777b03d25 in Interpret(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:3084:18
#22 0x7fd777aef981 in js::RunScript(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:435:12
#23 0x7fd777b0e3b0 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:513:15
#24 0x7fd777b0f2c5 in InternalCall(JSContext*, js::AnyInvokeArgs const&) /src/js/src/vm/Interpreter.cpp:540:12
#25 0x7fd777b0f4dc in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /src/js/src/vm/Interpreter.cpp:559:10
#26 0x7fd7783c4c9b in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /src/js/src/jsapi.cpp:2965:12
#27 0x7fd772623cd5 in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) /src/obj-firefox/dom/bindings/EventHandlerBinding.cpp:260:37
#28 0x7fd772e8dccb in void mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) /src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:362:12
#29 0x7fd772e8c4fb in mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) /src/dom/events/JSEventHandler.cpp:215:12
#30 0x7fd772e677bf in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) /src/dom/events/EventListenerManager.cpp:1112:51
#31 0x7fd772e68ac5 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) /src/dom/events/EventListenerManager.cpp:1283:20
#32 0x7fd772e553db in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /src/dom/events/EventDispatcher.cpp:313:17
#33 0x7fd772e54a9f in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /src/dom/events/EventDispatcher.cpp:462:16
#34 0x7fd772e5657d in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /src/dom/events/EventDispatcher.cpp:822:9
#35 0x7fd77497ebd0 in nsDocumentViewer::LoadComplete(nsresult) /src/layout/base/nsDocumentViewer.cpp:1081:7
#36 0x7fd776eccdcd in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) /src/docshell/base/nsDocShell.cpp:7741:21
#37 0x7fd776eca4aa in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /src/docshell/base/nsDocShell.cpp:7539:7
#38 0x7fd776ece51f in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /src/docshell/base/nsDocShell.cpp:7436:13
#39 0x7fd7704d2a24 in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) /src/uriloader/base/nsDocLoader.cpp:1320:3
#40 0x7fd7704d2009 in nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) /src/uriloader/base/nsDocLoader.cpp:861:14
#41 0x7fd7704cfb31 in nsDocLoader::DocLoaderIsEmpty(bool) /src/uriloader/base/nsDocLoader.cpp:750:9
#42 0x7fd7704d125c in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /src/uriloader/base/nsDocLoader.cpp:632:5
#43 0x7fd7704d1bcc in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /src/uriloader/base/nsDocLoader.cpp:488:14
#44 0x7fd76f0869c0 in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) /src/netwerk/base/nsLoadGroup.cpp:629:28
#45 0x7fd771375fc7 in nsDocument::DoUnblockOnload() /src/dom/base/nsDocument.cpp:9171:18
#46 0x7fd771375d2b in nsDocument::UnblockOnload(bool) /src/dom/base/nsDocument.cpp:9093:9
#47 0x7fd77135bc26 in nsDocument::DispatchContentLoadedEvents() /src/dom/base/nsDocument.cpp:5597:3
#48 0x7fd7713d4465 in mozilla::detail::RunnableMethodImpl<nsDocument*, void (nsDocument::*)(), true, (mozilla::RunnableKind)0>::Run() /src/obj-firefox/dist/include/nsThreadUtils.h:1192:13
#49 0x7fd76eee542f in nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1039:14
#50 0x7fd76eeea7a0 in NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:521:10
#51 0x7fd76fa5b575 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21
#52 0x7fd76f9b04b7 in MessageLoop::RunInternal() /src/ipc/chromium/src/base/message_loop.cc:326:10
#53 0x7fd76f9b0349 in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299:3
#54 0x7fd77436c64a in nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:158:27
#55 0x7fd777588021 in nsAppStartup::Run() /src/toolkit/components/startup/nsAppStartup.cpp:288:30
#56 0x7fd7776ec4b1 in XREMain::XRE_mainRun() /src/toolkit/xre/nsAppRunner.cpp:4701:22
#57 0x7fd7776ee17a in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4865:8
#58 0x7fd7776ef068 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4960:21
#59 0x4ed398 in do_main(int, char**, char**) /src/browser/app/nsBrowserApp.cpp:236:22
#60 0x4eccb0 in main /src/browser/app/nsBrowserApp.cpp:309:16
#61 0x7fd78e05e82f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
#62 0x41e9e4 in _start (/home/user/workspace/browsers/m-c-1505595111-asan-debug/firefox+0x41e9e4)
Flags: in-testsuite?
Ehsan, can you please take a look here?
Flags: needinfo?(ehsan)
Assignee: nobody → ehsan
Priority: -- → P1
I'm trying to bisect this with mozregression and on older builds, it hits a different assertion:
Assertion failure: clearedEntries == 0 (Map should be empty already), at /home/worker/workspace/build/src/dom/base/DirectionalityUtils.cpp:551
This testcase asserts as far back as mozregression can find builds (1 year).

I was at least able to bisect the change in assertion:
INFO: Last good revision: b274e6e81c8bd890dade81be1e989226ccad9fd2
INFO: First bad revision: b99ff3a0012c57ffab1fa291234be3f4428d5875
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b274e6e81c8bd890dade81be1e989226ccad9fd2&tochange=b99ff3a0012c57ffab1fa291234be3f4428d5875
--> Bug 1346590
Sorry I didn't have a chance to look into this one this week, on my list next week.
FWIW, I think we should discuss what to do with DirectionalityMap in general. It has been a constant source of bugs for 5 years.
(In reply to Olli Pettay [:smaug] from comment #5)
> FWIW, I think we should discuss what to do with DirectionalityMap in
> general. It has been a constant source of bugs for 5 years.

This bug in particular was actually not the fault of DirectionalityMap, it was a logic error in our code.

Per spec, <bdi> is special with two regards:

 * If it doesn't have a dir=rtl, dir=ltr or dir=auto attribute, it should be treated as if it has a dir=auto attribute.
 * Otherwise, a text node under it shouldn't be used to determine the direction of an ancestor.

The bug was that when enforcing the second part, we were mistakenly looking at the dir attribute of <bdi> elements in some cases.

For example, when trying to resolve the directionality of an element, we would look for the correct text node in this function: <https://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/dom/base/DirectionalityUtils.cpp#384>.  Here, DoesNotAffectDirectionOfAncestors() is supposed to return true if we hit a node which should be ignored when looking for a text node to resolve the direction.  The HTML spec <https://html.spec.whatwg.org/multipage/dom.html#the-dir-attribute> says:

 * ...The character is not in a Text node that has an ancestor element that is a descendant of the element whose directionality is being determined and that is either:

    A bdi element.
    A script element.
    A style element.
    A textarea element.
    An element with a dir attribute in a defined state. 

DoesNotAffectDirectionOfAncestors() <https://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/dom/base/DirectionalityUtils.cpp#262> would check for script, style and textarea in DoesNotParticipateInAutoDirection(), would check for the last statement with |aElement->HasFixedDir()| but when checking for <bdi> elements it would use IsBdiWithoutDirAuto().  In the test case for this bug, the <bdi> element has a dir=auto attribute so we would mistakenly pick the text node under it as the text node that determines the directionality of <body>.

Elsewhere <https://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/dom/base/DirectionalityUtils.cpp#1013> we would check for all <bdi> nodes correctly which would cause things to end up in an inconsistent state.
Flags: needinfo?(ehsan)
Comment on attachment 8911936 [details] [diff] [review]
Don't use text nodes under <bdi> elements to determine the directionality of the ancestors

Hmm, I need to read the spec again.
Attachment #8911936 - Flags: review?(bugs) → review+
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbd8e9aa0a0f
Don't use text nodes under <bdi> elements to determine the directionality of the ancestors; r=smaug
FWIW I don't think this is worth backporting.  We have been shipping this bug for a very long time (probably since the initial dir=auto support) and I prefer the patch to ride the trains as usual.
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/fbd8e9aa0a0f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
See Also: → 1414452
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.