Closed Bug 1414452 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox-esr52 58+ verified
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 + verified
firefox59 + verified

People

(Reporter: tsmith, Assigned: smaug)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, sec-high, testcase, Whiteboard: [adv-main58+][adv-esr52.6+])

Attachments

(2 files)

Attached file 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)
Flags: in-testsuite?
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.
Has Regression Range: --- → no
Version: 58 Branch → unspecified
Group: dom-core-security
Do we have anything to do here, then?
Flags: needinfo?(bugs)
I need to take a look what is happening here. This is worrisome, the whole nsTextNodeDirectionalityMap code is worrisome.
Assignee: nobody → bugs
Flags: needinfo?(bugs)
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.
Attachment #8931128 - Flags: review?(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?
Attachment #8931128 - Flags: review?(peterv) → review+
Priority: -- → P1
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 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
Attachment #8931128 - Flags: sec-approval?
Attachment #8931128 - Flags: approval-mozilla-esr52?
Attachment #8931128 - Flags: approval-mozilla-beta?
Comment on attachment 8931128 [details] [diff] [review]
safer_EnsureMapIsClear.diff

sec-approval+ and I'll give beta approval as well.
Attachment #8931128 - Flags: sec-approval?
Attachment #8931128 - Flags: sec-approval+
Attachment #8931128 - Flags: approval-mozilla-beta?
Attachment #8931128 - Flags: approval-mozilla-beta+
Why is ESR52 marked "won't fix" for this?
Because the sec implications weren't clear at the time.
Target Milestone: --- → mozilla59
Group: dom-core-security → core-security-release
Comment on attachment 8931128 [details] [diff] [review]
safer_EnsureMapIsClear.diff

Sec-high, ESR52+
Attachment #8931128 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [adv-main58+][adv-esr52.6+]
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.