SVG tearoffs don't remove themselves from hashtables when unlinked
Categories
(Core :: SVG, defect, P2)
Tracking
()
People
(Reporter: tsmith, Assigned: heycam)
References
(Blocks 1 open bug)
Details
(Keywords: assertion, testcase, Whiteboard: [fuzzblocker])
Attachments
(2 files)
The testcase requires a fuzzing build to trigger GC and CC. Assertion failure: cache->PreservingWrapper(), at src/dom/bindings/DOMJSProxyHandler.cpp:86 0|0|libxul.so|mozilla::dom::CheckExpandoObject|hg:hg.mozilla.org/mozilla-central:dom/bindings/DOMJSProxyHandler.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|78|0x0 0|1|libxul.so|mozilla::dom::DOMProxyHandler::GetExpandoObject(JSObject*)|hg:hg.mozilla.org/mozilla-central:dom/bindings/DOMJSProxyHandler.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|326|0xb 0|2|libxul.so|mozilla::dom::SVGPointList_Binding::DOMProxyHandler::getOwnPropDescriptor(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, bool, JS::MutableHandle<JS::PropertyDescriptor>) const|s3:gecko-generated-sources:94db611292a03fde03683071a2635ad8c34dc4256542e176fef73bb26fc9ff3624f978ba375ef9ab48189214064b557902079aba9d9291c10920a2db4c79dd3a/dom/bindings/SVGPointListBinding.cpp:|657|0x5 0|3|libxul.so|mozilla::dom::DOMProxyHandler::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) const|hg:hg.mozilla.org/mozilla-central:dom/bindings/DOMJSProxyHandler.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|265|0x6 0|4|libxul.so|js::Proxy::setInternal(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&)|hg:hg.mozilla.org/mozilla-central:js/src/proxy/Proxy.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|440|0x23 0|5|libxul.so|js::Proxy::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&)|hg:hg.mozilla.org/mozilla-central:js/src/proxy/Proxy.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|450|0x19 0|6|libxul.so|js::SetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&)|hg:hg.mozilla.org/mozilla-central:js/src/vm/ObjectOperations-inl.h:a12d80e08655c13245add6f6dacc91f8a6d9cf89|295|0x5 0|7|libxul.so|SetObjectElementOperation|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|1888|0x8 0|8|libxul.so|js::SetObjectElement(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, bool, JS::Handle<JSScript*>, unsigned char*)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|5258|0x22 0|9|libxul.so|js::jit::DoSetElemFallback|hg:hg.mozilla.org/mozilla-central:js/src/jit/BaselineIC.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|2022|0x5 0|10|||||0xa9417cb0eed 0|11|||||0xa9417c9c7e5 0|12|libxul.so|EnterJit|hg:hg.mozilla.org/mozilla-central:js/src/jit/Jit.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|105|0x29 0|13|libxul.so|Interpret|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|3537|0xb 0|14|libxul.so|js::RunScript(JSContext*, js::RunState&)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|447|0xb 0|15|libxul.so|js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|587|0xf 0|16|libxul.so|InternalCall|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|614|0xd 0|17|libxul.so|js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|634|0x5 0|18|libxul.so|JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>)|hg:hg.mozilla.org/mozilla-central:js/src/jsapi.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|2996|0x1c 0|19|libxul.so|mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&)|s3:gecko-generated-sources:344e7bbc1d2eb535f4d85b157b525fa18eb1e4688bb70bf6e3c51b02fe597784d3fe3e6a2b078d2e5b5c857d2435e216ad8a578cae88f140c6e1c2a969833ac5/dom/bindings/EventHandlerBinding.cpp:|265|0x5 0|20|libxul.so|mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*)|s3:gecko-generated-sources:3760fb8938acb537e28af191191b3b28eae59c21a2b0dcc49f0990b2613d2b29a3f705864d9a9ad5a160e5faa013e2d0b447215b3fc0b25795d7ebbd0dc4ce29/dist/include/mozilla/dom/EventHandlerBinding.h:|363|0x1f 0|21|libxul.so|mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*)|hg:hg.mozilla.org/mozilla-central:dom/events/EventListenerManager.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|1115|0x14 0|22|libxul.so|mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool)|hg:hg.mozilla.org/mozilla-central:dom/events/EventListenerManager.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|1317|0x16 0|23|libxul.so|mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&)|hg:hg.mozilla.org/mozilla-central:dom/events/EventListenerManager.h:a12d80e08655c13245add6f6dacc91f8a6d9cf89|391|0x6 0|24|libxul.so|mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&)|hg:hg.mozilla.org/mozilla-central:dom/events/EventDispatcher.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|642|0x12 0|25|libxul.so|mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*)|hg:hg.mozilla.org/mozilla-central:dom/events/EventDispatcher.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|1165|0x1a 0|26|libxul.so|AsyncTimeEventRunner::Run|hg:hg.mozilla.org/mozilla-central:dom/smil/nsSMILTimedElement.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|111|0x1a 0|27|libxul.so|nsThread::ProcessNextEvent(bool, bool*)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThread.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|1244|0x11 0|28|libxul.so|NS_ProcessNextEvent(nsIThread*, bool)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThreadUtils.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|530|0x11 0|29|libxul.so|mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|97|0xa 0|30|libxul.so|MessageLoop::RunInternal()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:a12d80e08655c13245add6f6dacc91f8a6d9cf89|325|0x17 0|31|libxul.so|MessageLoop::Run()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:a12d80e08655c13245add6f6dacc91f8a6d9cf89|318|0x8 0|32|libxul.so|nsBaseAppShell::Run()|hg:hg.mozilla.org/mozilla-central:widget/nsBaseAppShell.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|158|0xd 0|33|libxul.so|nsAppStartup::Run()|hg:hg.mozilla.org/mozilla-central:toolkit/components/startup/nsAppStartup.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|290|0xe 0|34|libxul.so|XREMain::XRE_mainRun()|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsAppRunner.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|4791|0x11 0|35|libxul.so|XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&)|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsAppRunner.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|4936|0x8 0|36|libxul.so|XRE_main(int, char**, mozilla::BootstrapConfig const&)|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsAppRunner.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|5028|0x5 0|37|firefox-bin|do_main|hg:hg.mozilla.org/mozilla-central:browser/app/nsBrowserApp.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|233|0x22 0|38|firefox-bin|main|hg:hg.mozilla.org/mozilla-central:browser/app/nsBrowserApp.cpp:a12d80e08655c13245add6f6dacc91f8a6d9cf89|315|0xd 0|39|libc-2.23.so||||0x20830 0|40|firefox-bin|MOZ_ReportAssertionFailure|hg:hg.mozilla
Updated•5 years ago
|
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 6•5 years ago
|
||
This looks like a bug in DOMSVGPointList to me.
The sequence of events here is as follows:
-
We execute
b.max = 0.44
. This triggerseh()
which sets an expando ona.animatedPoints
and preserves its wrapper. -
We GC and CC. Since none of the SVG code is holding a strong ref to the DOMSVGPointList we decide it's part of a garbage cycle (with its wrapper) and unlink it. That un-preserves the wrapper. The DOMSVGPointList does NOT get destroyed at this point; that will happen after the wrapper is GCed, drops its ref, and the snowwhite killer runs (probably async).
-
We try to touch a.animatedPoints again. This does a lookup on the tearoff hashtable, finds the now-unlinked object, and returns it.
To fix this specific assertion, it seems like we should at the very least remove ourselves from the tearoff hashtable in Unlink as well, not just in ~DOMSVGPointList. Returning unlinked objects to script is not good.
That said, that would still leave incorrect web-visible behavior: the object would lose its expandos. Really, we should be keeping that thing alive while the element is alive or something...
Comment 7•5 years ago
|
||
Also, I kinda doubt that the various intermittents blamed on this bug are the same bug. :( Let's see if we can stop that by changing the summary.
Comment 8•5 years ago
|
||
I filed bug 1542461 on the thing people have been seeing on treeherder.
Comment hidden (Intermittent Failures Robot) |
Updated•5 years ago
|
Comment 10•5 years ago
|
||
would fixing bug 1242048 fix this? Sadly I've never known what to do there as I don't understand what to do with bug 1242048 comment 12.
Comment 11•5 years ago
|
||
would fixing bug 1242048 fix this?
Um... it depends on how we fix it, exactly, no? If we fix by just removing the [Constant], then no.
I don't understand what to do with bug 1242048 comment 12.
Comment 12•5 years ago
|
||
Ugh, why did that get submitted?
Anyway, for purposes of this bug I suspect we should just remove from hashtable when unlinked. That keeps our current issue with us killing objects when we should not, which I guess bug 1242048 kinda sorta tracks.
I'm happy to talk about bug 1242048 comment 12 either in that bug, or via email, or on irc. Let me know.
Comment hidden (Intermittent Failures Robot) |
Reporter | ||
Comment 14•5 years ago
|
||
The fuzzers are still hitting this issue frequently.
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Sean - can I please get some help on this issue? It causes a lot of wasted CPU time when fuzzing SVG and DOM layout fuzzers.
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 20•5 years ago
|
||
I'm going to ni? heycam as well. We will discuss in upcoming triage meetings.
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Reporter | ||
Comment 30•4 years ago
|
||
The fuzzers are still hitting this frequently and it is also an intermittent failure.
It seems the issue is understood but I can get a Pernosco session if needed.
Comment 31•4 years ago
|
||
This is understood. It just needs to be addressed. Similar issue to what Kris just fixed for weakrefs, fwiw: someone getting their hands on an unlinked object is bad...
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 33•4 years ago
|
||
Assignee | ||
Comment 34•4 years ago
|
||
The only three types that can hit the PreservedWrapper() assertion
currentl, and which this patch avoids, are DOMSVGPathSegList, DOMSVGPointList,
and DOMSVGStringList.
This is because these are the only types that are implemented as proxies
(due to having e.g. indexed getters on them, and thus have the offending
DOMProxyHandler::GetExpandoObject() call in their bindings) and which we
store in tearoff tables.
Assignee | ||
Comment 35•4 years ago
|
||
Apologies for leaving this fuzz blocker open for so long, Tyson.
Comment 36•4 years ago
|
||
Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/39f8ceb567ec Remove SVG tearoff objects from the table when they are unlinked. r=longsonr
Comment 37•4 years ago
|
||
bugherder |
Comment 38•4 years ago
|
||
Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.
Reporter | ||
Comment 39•4 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #35)
Apologies for leaving this fuzz blocker open for so long, Tyson.
No problem. Thank you for your help :)
Updated•4 years ago
|
Description
•