Closed Bug 1403377 Opened 3 years ago Closed 3 years ago

Assertion failure: IsIdle(oldState) [@ PLDHashTable::RemoveEntry]

Categories

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

defect

Tracking

()

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

People

(Reporter: tsmith, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(2 files)

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

#0 Checker::StartWriteOp() /src/xpcom/ds/PLDHashTable.h:130:5
#1 PLDHashTable::RemoveEntry(PLDHashEntryHdr*) /src/xpcom/ds/PLDHashTable.cpp:648:15
#2 nsBaseHashtable<nsAttrHashKey, RefPtr<mozilla::dom::Attr>, mozilla::dom::Attr*>::LookupResult::Remove() /src/obj-firefox/dist/include/nsBaseHashtable.h:209:14
#3 nsDOMAttributeMap::DropAttribute(int, nsIAtom*) /src/dom/base/nsDOMAttributeMap.cpp:123:11
#4 mozilla::dom::Element::UnsetAttr(int, nsIAtom*, bool) /src/dom/base/Element.cpp:2904:27
#5 nsDOMAttributeMap::BlastSubtreeToPieces(nsINode*) /src/dom/base/nsDocument.cpp:7805:20
#6 nsIDocument::AdoptNode(nsINode&, mozilla::ErrorResult&) /src/dom/base/nsDocument.cpp:7988:5
#7 AdoptNodeIntoOwnerDoc(nsINode*, nsINode*, mozilla::ErrorResult&) /src/dom/base/nsINode.cpp:1524:42
#8 nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) /src/dom/base/nsINode.cpp:2443:5
#9 mozilla::dom::NodeBinding::appendChild(JSContext*, JS::Handle<JSObject*>, nsINode*, JSJitMethodCallArgs const&) /src/obj-firefox/dom/bindings/NodeBinding.cpp:885:45
#10 mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /src/dom/bindings/BindingUtils.cpp:3055:13
#11 js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /src/js/src/jscntxtinlines.h:293:15
#12 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:495:16
#13 InternalCall(JSContext*, js::AnyInvokeArgs const&) /src/js/src/vm/Interpreter.cpp:540:12
#14 js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /src/js/src/jit/BaselineIC.cpp:2589:14
#15 0x23e639502b21  (<unknown module>)
Flags: in-testsuite?
Summary: Assertion failure: IsIdle(oldState) with testcase that exhausts the stack while adopting → Assertion failure: IsIdle(oldState) [@ PLDHashTable::RemoveEntry]
This goes back further than mozregression can bisect (~1yr) :(
I think this is a bogus assert, perhaps due to bug 1187782.

As the comment in BlastSubtreeToPieces says:
  // This non-standard style of iteration is presumably used because some
  // of the code in the loop body can trigger element removal, which
  // invalidates the iterator.

The important part of the blast method is this:
  auto iter = map->mAttributeCache.ConstIter();
  ...
  nsCOMPtr<nsIAttribute> attr = iter.UserData();
  ...
  <some code that removes an element from the hash table>

Creating an iterator marks the hash table as active. Then the code grabs the data in the iterator, but it is just doing this to grab an arbitrary element. Then we remove something from the hash table, which causes the assertion failure when it checks if the hash table is in use. However, it isn't really in use, because |iter| is dead at this point.

The fix is to scope |iter| more tightly so that it is destroyed by the time we remove something from the hash table. It is unfortunate that we don't already have a test for this in the tree.
Assignee: nobody → continuation
I'll mark this sec-other for now, but it can be unhidden once somebody double checks my logic.
Keywords: sec-other
Nathan, does my reasoning sound right to you?
Flags: needinfo?(nfroyd)
I'll try to reproduce this, since there is another BlastSubtreeToPieces bug which I haven't managed to reproduce and these could be similar issues.
(In reply to Andrew McCreight [:mccr8] from comment #4)
> Nathan, does my reasoning sound right to you?

This logic sounds correct; the element->UnsetAttr call is guaranteed to remove the attribute from the hashtable we're iterating over?
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #6)
> This logic sounds correct; the element->UnsetAttr call is guaranteed to
> remove the attribute from the hashtable we're iterating over?

Yes, I think so.
Group: dom-core-security
Keywords: sec-other
Severity: critical → normal
Priority: -- → P2
Comment on attachment 8912926 [details]
Bug 1403377 - Destroy hash table iterator before modifying hash table.

https://reviewboard.mozilla.org/r/184242/#review189720
Attachment #8912926 - Flags: review?(bugs) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 5092af0f6bfa -d f215f88437f1: rebasing 423163:5092af0f6bfa "Bug 1403377 - Destroy hash table iterator before modifying hash table. r=smaug" (tip)
merging dom/base/crashtests/crashtests.list
merging dom/base/nsDocument.cpp
warning: conflicts while merging dom/base/crashtests/crashtests.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e003f2efac27
Destroy hash table iterator before modifying hash table. r=smaug
https://hg.mozilla.org/mozilla-central/rev/e003f2efac27
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Can this ride the 58 train?
Flags: needinfo?(continuation)
Flags: in-testsuite?
Flags: in-testsuite+
(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)
> Can this ride the 58 train?

Yeah, this should have no affect on release builds. The only reason to uplift it would be if there's a lot of fuzzing of 57 and this error gets hit a lot there.
Flags: needinfo?(continuation)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.