Crash report: https://crash-stats.mozilla.org/report/index/2d8f41fd-8e8b-4de5-99b4-dba330220304

MOZ_CRASH Reason: MOZ_CRASH(GFX: This should never be called without a context)

Top 10 frames of crashing thread:

0 libxul.so mozilla::SVGFilterObserverListForCanvasContext::OnRenderingChange layout/svg/SVGObserverUtils.cpp:867
1 libxul.so mozilla::SVGFilterObserver::OnRenderingChange layout/svg/SVGObserverUtils.cpp:748
2 libxul.so mozilla::SVGIDRenderingObserver::ElementTracker::ElementChanged layout/svg/SVGObserverUtils.cpp:357
3 libxul.so mozilla::dom::IDTracker::ChangeNotification::Run dom/base/IDTracker.h:140
4 libxul.so nsContentUtils::RemoveScriptBlocker dom/base/nsContentUtils.cpp:5696
5 libxul.so mozilla::dom::Document::cycleCollection::Unlink dom/base/Document.cpp:2652
6 libxul.so nsCycleCollector::CollectWhite xpcom/base/nsCycleCollector.cpp:3074
7 libxul.so nsCycleCollector::Collect xpcom/base/nsCycleCollector.cpp:3438
8 libxul.so nsCycleCollector_collectSlice xpcom/base/nsCycleCollector.cpp:3925
9 libxul.so mozilla::CCGCScheduler::CCRunnerFired dom/base/nsJSEnvironment.cpp:1572

I just crashed with this signature a handful of times with a testcase that I was writing locally to exercise a (not-intended-to-be-crashy) SVG-filter-in-canvas rendering scenario.

Seems to be a null-deref via MOZ_CRASH, so I'm assuming it's not security-sensitive for the time being.

The crash is here, with mContext apparently being null, given that we're hitting the MOZ_CRASH:

void SVGFilterObserverListForCanvasContext::OnRenderingChange() {
  if (!mContext) {
    MOZ_CRASH("GFX: This should never be called without a context");
  }

https://searchfox.org/mozilla-central/rev/d6850ecec549f9a1b10dd8ec8823db3791cf81fd/layout/svg/SVGObserverUtils.cpp#865-868

The only way mContext would get assigned to null would be via DetachFromContext() which gets called by SVGObserverUtils::DetachFromCanvasContext, which gets called as part of NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(CanvasRenderingContext2D):
https://searchfox.org/mozilla-central/rev/d6850ecec549f9a1b10dd8ec8823db3791cf81fd/dom/canvas/CanvasRenderingContext2D.cpp#809,826

And indeed: as shown in the backtrace, we are in the process of cycle collection when we crash, so we've probably triggered this "unlink" code and triggered this Detach*** mContext-nulling-out codepath.

Pernosco trace: https://pernos.co/debug/gw3A9HI_sgEhYgolYGCgMw/index.html

So what seems to be going on here is:

1. Cycle collection starts.
2. We unlink the CanvasRenderingContext2D which makes us call SVGObserverUtils::DetachFromCanvasContext as hinted in comment 1, which sets mContext to nullptr.

Here's a pernosco link to this moment in step 2.

3. Cycle collection proceeds to call Unlink() on the SVGDocument.
4. As part of that, we call UnbindFromTree on the SVGSVGElement, and recursively on all of its descendants, including the SVGFilterElement.
5. As part of unbinding the filter element, we remove it from the ID table, calling Document::RemoveFromIdTable.
6. As part of that removal, we dispatch a IDTracker::ChangeNotification::ChangeNotification runnable for the fact that our ID-having-thing has been removed.

Here's a pernosco link to this moment (for the backtrace represented in steps 3-6 here).

7. A short time later, that runnable fires with the backtrace shown in comment 0, and we crash because the runnable expects mContext to be non-null.

Here's a pernosco link to this moment (one line before we crash).

I suspect we simply don't want to dispatch our ChangeNotification in this scenario where we're in the process of being cycle-collected.

Conveniently, we do have an InUnlinkOrDeletion() flag and API on the document -- maybe we should just check that before dispatching.
https://searchfox.org/mozilla-central/rev/d4ebb53e719b913afdbcf7c00e162f0e96574701/dom/base/Document.h#3120

Alternately: I can imagine there might be scenarios where the SVG document gets torn down where we do in fact need to dispatch a notification to a listener in another not-being-destroyed doc, who is referencing an ID in our doc. (I'm not entirely sure if this is possible, but I'm also not entirely sure it's not possible.)

I suppose the real root problem here is that the canvas (and its CanvasRenderingContext2D) is being destroyed at a time when we happen to queue up a change-notification. That's probably what we should check for and handle gracefully.

Perhaps it's simplest to just remove the MOZ_CRASH in OnRenderingChange(), and replace it with a graceful NS_WARNING + return.

Try run with the crashtest but not the fix, which hopefully might fail (I'm not sure how reliably the test fails; I had to run it in --run-until-failure locally to get it to fail. (I also was using a nonzero setTimeout which I reduced to 0 for the purpose of the final patch, though I'm not sure if that matters.):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5064cc038e9a7cf1801c2f574cf4507bb4158a3

Regardless of whether the test fails on Try, I confirmed that I can make it fail locally (without the fix) by running under chaos mode:

MOZ_CHAOSMODE=1 ./mach crashtest --run-until-failure layout/svg/crashtests/1758029-1.html

In my debug+opt build, it failed on the 6th iteration one time and on the 4th iteration the next (less than 10 seconds from the time I hit my enter key).

So: I think that means the test can fail, with appropriately-timed cycle collection. So if this were to regress, the test would probably catch it and start failing eventually.

https://hg.mozilla.org/mozilla-central/rev/12e1eb49424d