1865148 - CookieBannerChild cleanup timeout never executes on some sites

Status: RESOLVED FIXED

(Core :: Privacy: Anti-Tracking, defect, P3)

Description

[Emma Zühlcke [:emz]]

I've observed this issue while testing a broken rule on https://futurezone.de
The timeout gets scheduled here with a 5 second delay, but is never executed.
Never executing the cleanup callback means that we keep polling for cookie banners indefinitely.

We use Timer.sys.mjs for setTimeout. When I switched over to use this.contentWindow.setTimeout the callback was executed successfully. The bug might be in the Timer.sys.mjs implementation.

Comment 1

[Emma Zühlcke [:emz]]

Lowering priority now that we have a fixup in place (Bug 1865149).

Comment 2

[Emma Zühlcke [:emz]]

Bug 1865149 seems to work well and I don't have time to look into this further currently.

Comment 3

[majaharding]

I found the cause of this bug.

When the cleanup() function inside #promiseObserve() gets called with the result of the banner search, it cancels the observerCleanUpTimer, which can then no longer clean up the Button observer. The rules for https://futurezone.de match a banner on the page but not a button, hence why the bug shows up there.

Because the "load" event causes the cleanup timer to restart, the bug doesn't occur if "load" happens after "Starting to detect the target button".

Using this.contentWindow?.setTimeout() seemed to help, because the code to cancel that timeout was still lazy.clearTimeout(). i.e. The cleanup() function couldn't cancel the timeout and cause the bug.

I have a patch for this which fixes the bug and avoids the timing issues, I'll upload it shortly.

Comment 4

[majaharding]

Attachment: Bug 1865148 - Fix CookieBannerChild.sys.mjs killing its cleanup timout prematurely r=pbz

When the cleanup() function inside #promiseObserve() gets called with the result of the banner search,
it cancels the observerCleanUpTimer, which can then no longer clean up the Button observer.

I fixed this, and also other potential async timing issues, by stopping the cleanup function from cancelling
the timeout, and having the timeout set a persistent "givenUp" flag instead of trying to directly cancel the
promiseObserver.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47562b188068
Fix CookieBannerChild.sys.mjs killing its cleanup timout prematurely r=pbz

Comment 6

[Sandor Molnar[:smolnar]]

Backed out for causing perma mochistest failures @ test_event_listener_leaks.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/32310605dbc657ffb394fe09a0d9fa4c96c223df

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | dom/indexedDB/test/test_event_listener_leaks.html

Comment 7

[majaharding]

Attachment: Bug 1865148 - Use AbortControllers for destroying and timing out r=pbz

Previously, some tests were failing because the interval function was not immediately
cleaned up when didDestroy() was called, which kept some references around and prevented
objects from being garbage collected.

I solved this by recognising that there are two ways that a promiseObserve can exit without
success: when the timeout expires, and when the CookieBannerChild is destroyed. These need
to be handled differently, so I gave each case a different AbortController.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ce90d5b5b94
Fix CookieBannerChild.sys.mjs killing its cleanup timout prematurely r=pbz
https://hg.mozilla.org/integration/autoland/rev/f566974ded62
Use AbortControllers for destroying and timing out r=pbz

Comment 9

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/0ce90d5b5b94
https://hg.mozilla.org/mozilla-central/rev/f566974ded62