Closed Bug 1513603 Opened 5 years ago Closed 4 years ago

SVG tearoffs don't remove themselves from hashtables when unlinked

Categories

(Core :: SVG, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla77
Tracking Status
firefox-esr68 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox73 --- wontfix
firefox74 --- wontfix
firefox75 --- wontfix
firefox76 --- wontfix
firefox77 --- fixed

People

(Reporter: tsmith, Assigned: heycam)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [fuzzblocker])

Attachments

(2 files)

Attached file testcase.html
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
Flags: in-testsuite?
Priority: -- → P3

This looks like a bug in DOMSVGPointList to me.

The sequence of events here is as follows:

  1. We execute b.max = 0.44. This triggers eh() which sets an expando on a.animatedPoints and preserves its wrapper.

  2. 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).

  3. 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...

Component: DOM: Bindings (WebIDL) → SVG
Flags: needinfo?(jwatt)

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.

Summary: Assertion failure: cache->PreservingWrapper(), at src/dom/bindings/DOMJSProxyHandler.cpp:86 → SVG tearoffs don't remove themselves from hashtables when unlinked

I filed bug 1542461 on the thing people have been seeing on treeherder.

Flags: needinfo?(longsonr)

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.

Flags: needinfo?(longsonr) → needinfo?(bzbarsky)

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.

Flags: needinfo?(bzbarsky)

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.

The fuzzers are still hitting this issue frequently.

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.

Flags: needinfo?(svoisen)

I'm going to ni? heycam as well. We will discuss in upcoming triage meetings.

Flags: needinfo?(svoisen) → needinfo?(cam)
Whiteboard: [fuzzblocker] → [fuzzblocker][layout:triage-discuss]

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.

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...

Flags: needinfo?(cam)
Priority: P3 → P2
Assignee: nobody → cam
Status: NEW → ASSIGNED
Flags: needinfo?(jwatt)
Whiteboard: [fuzzblocker][layout:triage-discuss] → [fuzzblocker]

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.

Apologies for leaving this fuzz blocker open for so long, Tyson.

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
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

(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 :)

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: