1414452 - Assertion failure: IsIdle(oldState), at /build/src/xpcom/ds/PLDHashTable.h:132

Status: VERIFIED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Assertion failure: IsIdle(oldState), at /build/src/xpcom/ds/PLDHashTable.h:132

#0 Checker::StartWriteOp() /build/src/xpcom/ds/PLDHashTable.h:130:5
#1 PLDHashTable::Remove(void const*) /build/src/xpcom/ds/PLDHashTable.cpp:631:15
#2 mozilla::nsTextNodeDirectionalityMap::RemoveEntryForProperty(mozilla::dom::Element*) /build/src/dom/base/DirectionalityUtils.cpp:481:17
#3 mozilla::nsTextNodeDirectionalityMap::nsTextNodeDirectionalityMapPropertyDestructor(void*, nsAtom*, void*, void*) /build/src/dom/base/DirectionalityUtils.cpp:452:12
#4 nsPropertyTable::PropertyList::DeletePropertyFor(nsPropertyOwner) /build/src/dom/base/nsPropertyTable.cpp:296:5
#5 nsPropertyTable::DeleteProperty(nsPropertyOwner, nsAtom*) /build/src/dom/base/nsPropertyTable.cpp:232:23
#6 nsINode::DeleteProperty(unsigned short, nsAtom*) /build/src/dom/base/nsINode.cpp:195:41
#7 mozilla::nsTextNodeDirectionalityMap::ClearEntry(nsPtrHashKey<mozilla::dom::Element>*, void*) /build/src/dom/base/DirectionalityUtils.cpp:555:15
#8 nsCheapSet<nsPtrHashKey<mozilla::dom::Element> >::EnumerateEntries(nsCheapSetOperator (*)(nsPtrHashKey<mozilla::dom::Element>*, void*), void*) /build/src/xpcom/ds/nsCheapSets.h:88:15
#9 mozilla::nsTextNodeDirectionalityMap::EnsureMapIsClear(nsINode*) /build/src/dom/base/DirectionalityUtils.cpp:575:17
#10 nsTextNode::UnbindFromTree(bool, bool) /build/src/dom/base/nsTextNode.cpp:155:3
#11 mozilla::dom::Element::UnbindFromTree(bool, bool) /build/src/dom/base/Element.cpp:2090:37
#12 nsGenericHTMLElement::UnbindFromTree(bool, bool) /build/src/dom/html/nsGenericHTMLElement.cpp:537:20
#13 nsINode::doRemoveChildAt(unsigned int, bool, nsIContent*, nsAttrAndChildArray&) /build/src/dom/base/nsINode.cpp:1950:9
#14 mozilla::dom::FragmentOrElement::RemoveChildAt(unsigned int, bool) /build/src/dom/base/FragmentOrElement.cpp:1336:5
#15 nsINode::RemoveChild(nsINode&, mozilla::ErrorResult&) /build/src/dom/base/nsINode.cpp:619:3
#16 mozilla::dom::NodeBinding::removeChild(JSContext*, JS::Handle<JSObject*>, nsINode*, JSJitMethodCallArgs const&) /build/src/obj-firefox/dom/bindings/NodeBinding.cpp:1012:45
#17 mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /build/src/dom/bindings/BindingUtils.cpp:3040:13
#18 js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /build/src/js/src/jscntxtinlines.h:291:15
#19 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /build/src/js/src/vm/Interpreter.cpp:472:16
#20 InternalCall(JSContext*, js::AnyInvokeArgs const&) /build/src/js/src/vm/Interpreter.cpp:521:12
#21 Interpret(JSContext*, js::RunState&) /build/src/js/src/vm/Interpreter.cpp:3066:18
#22 js::RunScript(JSContext*, js::RunState&) /build/src/js/src/vm/Interpreter.cpp:422:12
#23 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /build/src/js/src/vm/Interpreter.cpp:494:15
#24 InternalCall(JSContext*, js::AnyInvokeArgs const&) /build/src/js/src/vm/Interpreter.cpp:521:12
#25 js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /build/src/js/src/vm/Interpreter.cpp:540:10
#26 JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /build/src/js/src/jsapi.cpp:3019:12
#27 mozilla::dom::IdleRequestCallback::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::IdleDeadline&, mozilla::ErrorResult&) /build/src/obj-firefox/dom/bindings/WindowBinding.cpp:830:8
#28 mozilla::dom::IdleRequestCallback::Call(mozilla::dom::IdleDeadline&, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) /build/src/obj-firefox/dist/include/mozilla/dom/WindowBinding.h:633:12
#29 mozilla::dom::IdleRequest::IdleRun(nsPIDOMWindowInner*, double, bool) /build/src/dom/base/IdleRequest.cpp:66:14
#30 nsGlobalWindow::RunIdleRequest(mozilla::dom::IdleRequest*, double, bool) /build/src/dom/base/nsGlobalWindow.cpp:836:19
#31 nsGlobalWindow::ExecuteIdleRequest(mozilla::TimeStamp) /build/src/dom/base/nsGlobalWindow.cpp:866:21
#32 nsThread::ProcessNextEvent(bool, bool*) /build/src/xpcom/threads/nsThread.cpp:1037:14
#33 NS_ProcessNextEvent(nsIThread*, bool) /build/src/xpcom/threads/nsThreadUtils.cpp:513:10
#34 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /build/src/ipc/glue/MessagePump.cpp:97:21
#35 MessageLoop::RunInternal() /build/src/ipc/chromium/src/base/message_loop.cc:326:10
#36 MessageLoop::Run() /build/src/ipc/chromium/src/base/message_loop.cc:299:3
#37 nsBaseAppShell::Run() /build/src/widget/nsBaseAppShell.cpp:158:27
#38 nsAppStartup::Run() /build/src/toolkit/components/startup/nsAppStartup.cpp:288:30
#39 XREMain::XRE_mainRun() /build/src/toolkit/xre/nsAppRunner.cpp:4675:22
#40 XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /build/src/toolkit/xre/nsAppRunner.cpp:4837:8
#41 XRE_main(int, char**, mozilla::BootstrapConfig const&) /build/src/toolkit/xre/nsAppRunner.cpp:4932:21
#42 do_main(int, char**, char**) /build/src/browser/app/nsBrowserApp.cpp:231:22
#43 main /build/src/browser/app/nsBrowserApp.cpp:304:16
#44 __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
#45 _start (firefox+0x41ebe4)

Comment 1

[Ryan VanderMeulen (PTO, back July 27)]

Just like bug 1400701, this testcase hits a different assertion which goes back more than a year:
Assertion failure: clearedEntries == 0 (Map should be empty already), at /home/worker/workspace/build/src/dom/base/DirectionalityUtils.cpp:551

And like bug 1400701, the assertion changed to the current one when bug 1346590 landed.

Comment 2

[Andrew Overholt [:overholt]]

Do we have anything to do here, then?

Comment 3

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

I need to take a look what is happening here. This is worrisome, the whole nsTextNodeDirectionalityMap code is worrisome.

Comment 4

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

Attachment: safer_EnsureMapIsClear.diff

This is the simplest approach I could think, and as such should be safest for branches.
The testcase shows that the assertion in EnsureMapIsClear doesn't hold, and need to then fix that clearance to work in a safe way - so not during enumeration, but after that.
EnsureMapIsClear also had a useless param.

We really should rewrite the whole DirectionalityMapUtils. We just keep hacking around the bugs it has.

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 8931128 [details] [diff] [review]
safer_EnsureMapIsClear.diff

Review of attachment 8931128 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/DirectionalityUtils.cpp
@@ +552,3 @@
>    {
> +    AutoTArray<Element*, 8>* entries =
> +      static_cast<AutoTArray<Element*, 8>*>(aData);

Maybe add a typedef for the array type?

Comment 6

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

I'm not really a fan of array typedefs. They make it harder to think about lifetime management
(whether one has strong or raw pointer).

Comment 7

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

Comment on attachment 8931128 [details] [diff] [review]
safer_EnsureMapIsClear.diff

Approval Request Comment
[Feature/Bug causing the regression]:
Old code
[User impact if declined]:
crashes
[Is this code covered by automated tests?]:
No. We need to take the testcase here as a crashtest
[Has the fix been verified in Nightly?]:
Hasn't landed yet
[Needs manual test from QE? If yes, steps to reproduce]: 
Run the test in debug build
[List of other uplifts needed for the feature/fix]: NA
[Is the change risky?]:
Shouldn't be risky. We just call certain methods a tiny bit later in order to avoid
reentrance in hashtable code.
[Why is the change risky/not risky?]: see above
[String changes made/needed]: NA

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8931128 [details] [diff] [review]
safer_EnsureMapIsClear.diff

sec-approval+ and I'll give beta approval as well.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Why is ESR52 marked "won't fix" for this?

Comment 10

[Ryan VanderMeulen (PTO, back July 27)]

Because the sec implications weren't clear at the time.

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/157a15cd5ecc

Comment 12

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/releases/mozilla-beta/rev/88bd9da5a396

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/157a15cd5ecc

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8931128 [details] [diff] [review]
safer_EnsureMapIsClear.diff

Sec-high, ESR52+

Comment 15

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/releases/mozilla-esr52/rev/079356ed5317

Comment 16

[Emil Ghitta, QA [:emilghitta]]

I have managed to reproduce the issue mentioned in comment 0 using Firefox 58.0a1 (BuildId:20171103095933).

This issue is no longer reproducible using Firefox 59.0a1 (BuildId:20171228094453), 58.0b13 (BuildId:20171227090910) and 52.5.3 esr (BuildId:20171227194825) using Ubuntu 16.04 64bit Asan Debug builds.