1836824 - Assertion failure: watcher, at /builds/worker/workspace/obj-build/dist/include/mozilla/StateWatching.h:208

Status: VERIFIED FIXED

(Core :: Graphics: Canvas2D, defect, P3)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found while fuzzing m-c 20230529-c10ba3b6a8ec (--enable-debug --enable-fuzzing)

To reproduce via Grizzly Replay:

$ pip install fuzzfetch grizzly-framework
$ python -m fuzzfetch -d --fuzzing -n firefox
$ python -m grizzly.replay ./firefox/firefox testcase.html

Assertion failure: watcher, at /builds/worker/workspace/obj-build/dist/include/mozilla/StateWatching.h:208

#0 0x7f26493f5f04 in mozilla::WatchManager<mozilla::dom::RequestedFrameRefreshObserver>::Unwatch(mozilla::WatchTarget&, void (mozilla::dom::RequestedFrameRefreshObserver::*)()) /builds/worker/workspace/obj-build/dist/include/mozilla/StateWatching.h:208:5
#1 0x7f26493e7099 in mozilla::dom::RequestedFrameRefreshObserver::DetachFromRefreshDriver() /builds/worker/checkouts/gecko/dom/html/HTMLCanvasElement.cpp:283:5
#2 0x7f26493e6ff7 in mozilla::dom::HTMLCanvasElement::Destroy() /builds/worker/checkouts/gecko/dom/html/HTMLCanvasElement.cpp:489:37
#3 0x7f26493e7162 in mozilla::dom::HTMLCanvasElement::cycleCollection::Unlink(void*) /builds/worker/checkouts/gecko/dom/html/HTMLCanvasElement.cpp:498:8
#4 0x7f26456835e0 in nsCycleCollector::CollectWhite() /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:3141:26
#5 0x7f2645684ffa in nsCycleCollector::Collect(mozilla::CCReason, ccIsManual, js::SliceBudget&, nsICycleCollectorListener*, bool) /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:3507:26
#6 0x7f2645684ced in nsCycleCollector::ShutdownCollect() /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:3418:20
#7 0x7f2645686106 in nsCycleCollector::Shutdown(bool) /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:3717:5
#8 0x7f2645687b61 in nsCycleCollector_shutdown(bool) /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:4041:18
#9 0x7f26457c9456 in mozilla::ShutdownXPCOM(nsIServiceManager*) /builds/worker/checkouts/gecko/xpcom/build/XPCOMInit.cpp:673:3
#10 0x7f264d04d35c in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:663:16
#11 0x55df739c9526 in content_process_main /builds/worker/checkouts/gecko/browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
#12 0x55df739c9526 in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:375:18
#13 0x7f2659429d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#14 0x7f2659429e3f in __libc_start_main csu/../csu/libc-start.c:392:3
#15 0x55df739a07c8 in _start (/home/user/workspace/browsers/m-c-20230605094751-fuzzing-debug/firefox-bin+0x587c8) (BuildId: 12ad878cfaa70b2bc4a7191a0344fcaba161fd13)

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

The stack suggests this issue is more Canvas code related.

Comment 2

[Timothy Nikkel (:tnikkel)]

Hmm, is this the right testcase? I don't think the attached testcase will even create a HTMLCanvasElement.

Comment 3

[Tyson Smith [:tsmith]]

(In reply to Timothy Nikkel (:tnikkel) from comment #2)

Hmm, is this the right testcase? I don't think the attached testcase will even create a HTMLCanvasElement.

Oh good catch, thanks.

Comment 4

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Comment 5

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20230608152955-256876c3862b.
The bug appears to have been introduced in the following build range:

Start: 531a15ae09d49e29c270830e9b873aa625a5a8c2 (20230527212147)
End: 47f0f49b40361f9c5bde3a5691dc5d65ec1e0061 (20230528204615)
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=531a15ae09d49e29c270830e9b873aa625a5a8c2&tochange=47f0f49b40361f9c5bde3a5691dc5d65ec1e0061

Comment 6

[Timothy Nikkel (:tnikkel)]

Probably bug 1834049 in that range.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1834049

:bradwerth, since you are the author of the regressor, bug 1834049, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 8

[Brad Werth [:bradwerth] | Out until Feb 2025]

I will find a fix.

Comment 9

[Donal Meehan [:dmeehan]]

:bradwerth any updates on the investigation? Next week is the final week of Fx115 beta

Comment 10

[Brad Werth [:bradwerth] | Out until Feb 2025]

This is happening because a canvas lazily-creates its context, and HTMLCanvasElement::GetFrameCaptureState has no capture state when there is no context. In this testcase, RequestedFrameRefreshObserver::Register is called when there is no context, and then Unregister is called when it does. The assertion happens because we're unwatching some state that was never watched.

Possible fixes:

1. Eagerly create context. This is expensive and too big a change.
2. Create the state watch when the context is created. This requires the HTMLCanvasElement to be notified when the context is created. Maybe tricky.
3. Independently track whether or not we are watching state and only unwatch it if we watched it in the first place. This would work but it would allow HTMLCanvasElement to be in a state where it wasn't watching its owning element's capture state at all. That's would probably just hide other more serious problems.

I'll try to build a solution around idea 2, above.

Comment 11

[Brad Werth [:bradwerth] | Out until Feb 2025]

Attachment: Bug 1836824 Part 1: Make HTMLCanvasElement re-register its frame refresh driver when a context is updated.

Since HTMLCanvasElement has a lazily-created context, and the
RequestedFrameRefreshObserver will fail to watch the state when the
context is null, this patch adds a hook to try to re-register when
UpdateContext is called. The superclass calls UpdateContext after the
context is initially created.

Comment 12

[Brad Werth [:bradwerth] | Out until Feb 2025]

Attachment: Bug 1836824 Part 2: Add a crashtest of canvas with captureStream before getContext.

This changes the original fuzzer test to explicitly delete the added
canvas element, rather than trying to close the window. Confirmed that
this asserts without the changes in Part 1.

Depends on D181149

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/102bde84aa26
Part 1: Make HTMLCanvasElement re-register its frame refresh driver when a context is updated. r=jgilbert
https://hg.mozilla.org/integration/autoland/rev/d3ef9848c1a0
Part 2: Add a crashtest of canvas with captureStream before getContext. r=jgilbert

Comment 14

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/102bde84aa26
https://hg.mozilla.org/mozilla-central/rev/d3ef9848c1a0

Comment 15

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20230617092009-29e4ffb2c397.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:bradwerth, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox115 to wontfix.

For more information, please visit BugBot documentation.

Comment 17

[Brad Werth [:bradwerth] | Out until Feb 2025]

(In reply to BugBot [:suhaib / :marco/ :calixte] from comment #16)

The patch landed in nightly and beta is affected.
:bradwerth, is this bug important enough to require an uplift?

No need to uplift this. It only affects debug builds.