Closed Bug 1710283 Opened 3 years ago Closed 3 years ago

Crash in [@ mozilla::dom::DOMIntersectionObserver::Unobserve] with tampermonkey addon

Categories

(Core :: Layout, defect)

defect

Tracking

()

RESOLVED FIXED
90 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox88 --- wontfix
firefox89 + fixed
firefox90 --- fixed

People

(Reporter: aryx, Assigned: emilio)

Details

(Keywords: crash)

Crash Data

Attachments

(2 files)

~130 crashes for ~90 installs of Firefox 88.0/88.0.1

Code had landed in bug 1687358 for Firefox 87 but there are only single crash instances before Gecko 88.

Sean, can you check what changed related to this in Firefox 88?

Crash report: https://crash-stats.mozilla.org/report/index/ab6791ec-6a42-4897-a1db-f41c50210509

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::DOMIntersectionObserver::Unobserve dom/base/DOMIntersectionObserver.cpp:240
1 xul.dll mozilla::dom::HTMLImageElement::LazyLoadImageReachedViewport dom/html/HTMLImageElement.cpp:1307
2 xul.dll mozilla::dom::LazyLoadCallbackReachViewport dom/base/DOMIntersectionObserver.cpp:165
3 xul.dll mozilla::dom::DOMIntersectionObserver::Notify dom/base/DOMIntersectionObserver.cpp:751
4 xul.dll mozilla::dom::Document::NotifyIntersectionObservers dom/base/Document.cpp:15439
5 xul.dll mozilla::detail::RunnableMethodImpl< xpcom/threads/nsThreadUtils.h:1201
6 xul.dll mozilla::SchedulerGroup::Runnable::Run xpcom/threads/SchedulerGroup.cpp:143
7 xul.dll mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal xpcom/threads/TaskController.cpp:757
8 xul.dll mozilla::detail::RunnableFunction<`lambda at /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:135:7'>::Run xpcom/threads/nsThreadUtils.h:534
9 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp
Flags: needinfo?(sefeng)

There is a strong correlation with the Tampermonkey add-on and the issue hadn't been observed initially with the new Firefox 88.0 version. It might have started with the rollout of the latest Tampermonkey version: https://addons.mozilla.org/en-US/firefox/addon/tampermonkey/

Summary: Crash in [@ mozilla::dom::DOMIntersectionObserver::Unobserve] → Crash in [@ mozilla::dom::DOMIntersectionObserver::Unobserve] with tampermonkey addon
Component: DOM: Core & HTML → Layout

Hi, Tampermonkey dev here. Please tell me in case I can help to investigate the issue.

FWIW: Tampermonkey 4.13.6136 roll out started on 2021-04-30

2021-04-30: 9K+ updates
2021-05-01: 64K+
2021-05-02: 61K+
2021-05-03: 57K+
2021-05-04: 45K+
2021-05-05: 32K+
2021-05-06: 31K+
2021-05-07: 25K+
2021-05-08: 15K+
...

It is therefore not clear to me why the problems should only have started on May 6th. I haven't received any problem reports on this issue either.

Out of 183 reported crashes for Firefox 88.0.1 and 88.0 (based on 100+ installations), 162 have tampermonkey installed and 155 uBlock. The issue isn't bound to the latest versions of the add-ons because a comparison of two crashes found the following version:

  • firefox%40tampermonkey.net: 4.12.6132, 4.13.6136
  • uBlock0%40raymondhill.net: 1.35.0, 1.35.2
    First crash report for Firefox 88.0 submitted at 2021-05-06 22:52:22 UTC - might be triggered by an external change of web content.

It seems plausible for the doc to be unlinked, or for the element to be moved
to another document, in between the time observer notification is scheduled and
the time it runs. In that case, we'd be unregistered already anyways, so
there's nothing else to do.

Assignee: nobody → emilio
Status: NEW → ASSIGNED
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a990505a788
Null-check lazyload intersection observers on unregistration. r=sefeng

Comment on attachment 9221111 [details]
Bug 1710283 - Null-check lazyload intersection observers on unregistration. r=sefeng

Beta/Release Uplift Approval Request

  • User impact if declined: Crashes
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce: (There are no known STR)
  • List of other uplifts needed: none
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Null-check
  • String changes made/needed: none
Attachment #9221111 - Flags: approval-mozilla-beta?

Not only approval-mozilla-beta, but also approval-mozilla-release is needed I think?

Flags: needinfo?(sefeng)

Comment on attachment 9221111 [details]
Bug 1710283 - Null-check lazyload intersection observers on unregistration. r=sefeng

Beta/Release Uplift Approval Request

  • User impact if declined: see above
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce: no concrete steps
  • List of other uplifts needed: none
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): null-check
  • String changes made/needed: none
Attachment #9221111 - Flags: approval-mozilla-release?
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch

Emilio, your patch does not graft cleanly to beta, could you rebase it please? Thanks

Flags: needinfo?(emilio)
Attached patch Beta patchSplinter Review
Flags: needinfo?(emilio) → needinfo?(pascalc)

Comment on attachment 9221289 [details] [diff] [review]
Beta patch

Let's uplift to beta as the volume should tell us rapidly if this fixes the problem, thanks!

Flags: needinfo?(pascalc)
Attachment #9221289 - Flags: approval-mozilla-beta+
Attachment #9221111 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

The fix was effective and we have no crashes with this signature since 89 beta 11, thanks.

Comment on attachment 9221111 [details]
Bug 1710283 - Null-check lazyload intersection observers on unregistration. r=sefeng

Fx89 is in RC now, no need to take in 88.

Attachment #9221111 - Flags: approval-mozilla-release? → approval-mozilla-release-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: