Closed Bug 1659758 Opened 1 year ago Closed 1 year ago

Crash in [@ nsDocShellTreeOwner::AddChromeListeners]

Categories

(Core :: Printing: Output, defect, P1)

80 Branch
defect

Tracking

()

RESOLVED FIXED
82 Branch
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- unaffected
firefox79 --- unaffected
firefox80 --- wontfix
firefox81 + fixed
firefox82 --- fixed

People

(Reporter: philipp, Assigned: emilio)

Details

(Keywords: crash, regression, Whiteboard: [print2020_v81])

Crash Data

Attachments

(1 file)

This bug is for crash report bp-f122a7b6-f865-45e6-918d-4080d0200818.

Top 10 frames of crashing thread:

0 xul.dll nsDocShellTreeOwner::AddChromeListeners docshell/base/nsDocShellTreeOwner.cpp:809
1 xul.dll nsDocShellTreeOwner::OnProgressChange docshell/base/nsDocShellTreeOwner.cpp:663
2 xul.dll nsDocLoader::FireOnProgressChange uriloader/base/nsDocLoader.cpp:1267
3 xul.dll nsDocLoader::OnProgress uriloader/base/nsDocLoader.cpp:1148
4 xul.dll nsProgressNotificationProxy::OnProgress image/imgLoader.cpp:566
5 xul.dll mozilla::net::HttpChannelChild::DoOnProgress netwerk/protocol/http/HttpChannelChild.cpp:971
6 xul.dll mozilla::detail::RunnableFunction<`lambda at /builds/worker/checkouts/gecko/netwerk/protocol/http/HttpChannelChild.cpp:859:13'>::Run xpcom/threads/nsThreadUtils.h:577
7 xul.dll mozilla::SchedulerGroup::Runnable::Run xpcom/threads/SchedulerGroup.cpp:146
8 xul.dll mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal xpcom/threads/TaskController.cpp:512
9 xul.dll mozilla::detail::RunnableFunction<`lambda at /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:86:7'>::Run xpcom/threads/nsThreadUtils.h:577

this content crash signature (related to printing) is jumping up in firefox 80.0b - most of the crashing urls point towards some printing specific pages from various indian government domains, but nothing that would be publicly reproducible

Any chance we can get this crash on the radar to be addressed in 81 alongside the other printing crashes?

Flags: needinfo?(svoisen)

jwatt: Could this be a result of bug 1652270?

Severity: -- → S2
Flags: needinfo?(svoisen) → needinfo?(jwatt)
Priority: -- → P1
Whiteboard: [print2020_v81]

The crash report URLs/comments may indicate this is happening during printing, but there's nothing in the stack that would otherwise have led me to think that. It's not obvious to me how this relates to printing.

(In reply to Sean Voisen (:svoisen) from comment #2)

jwatt: Could this be a result of bug 1652270?

I don't think so. Apart from anything else it wasn't until 10 days after that bug landed that the increases in crashes starts to become noticable.

It looks like the GetDOMEventTarget() call in nsDocShellTreeOwner::AddChromeListeners isn't finding a target, and then we [crash trying to call GetOrCreateListenerManager on it](https://hg.mozilla.org/mozilla-central/file/b0d012ec753deb0880963f267806035da3a6b013/docshell/base/nsDocShellTreeOwner.cpp#l8090.

I don't know enought about this code to know whether failing to find a target indicates something bad, or if this could be expected and we just need to add a null check.

Smaug, could you maybe shed some light on that?

Flags: needinfo?(jwatt) → needinfo?(bugs)

I guess the relevant tab(?) is being closed when event listeners are trying to be added. Does some new printing code keep a http channel open for too long, even after the tab has been closed? Or has some progress listener alive longer?

Could this be from https://bugzilla.mozilla.org/show_bug.cgi?id=1648064 ?

Flags: needinfo?(bugs)
Flags: needinfo?(emilio)

This happens under the window.print() callstack, so I'd say is not too likely. The others one don't seem particularly related to printing...

Some other recentish changes like https://hg.mozilla.org/mozilla-central/rev/2e60bc5e273d sound like they could've introduced this... Those channels didn't end up notifying before dying before that patch, so maybe that's what increased the volume.

I think bailing out if the tab is being unloaded makes sense anyhow...

Tentative fix (though crash is long-standing), so no test :/

Assignee: nobody → emilio
Status: NEW → ASSIGNED

Without a repro, adding a null-check seems a decent-ish way to avoid crashing...

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/062c7597ed2c
Null-check event target in nsDocShellTreeOwner::AddChromeListeners. r=smaug

Comment on attachment 9171819 [details]
Bug 1659758 - Null-check event target in nsDocShellTreeOwner::AddChromeListeners. r=smaug

Beta/Release Uplift Approval Request

  • User impact if declined: Crashes (potentially related to printing, but some of them are really long-standing and it's not really clear whether print-related)
  • 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: we have no 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 instead of crash
  • String changes made/needed: none
Attachment #9171819 - Flags: approval-mozilla-beta?
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch

Comment on attachment 9171819 [details]
Bug 1659758 - Null-check event target in nsDocShellTreeOwner::AddChromeListeners. r=smaug

Can't hurt to try! Approved for 81.0b2.

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