1659758 - Crash in [@ nsDocShellTreeOwner::AddChromeListeners]

Status: RESOLVED FIXED

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

Description

[[:philipp]]

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

Comment 1

[Ryan VanderMeulen [:RyanVM]]

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

Comment 2

[Sean Voisen (:svoisen)]

jwatt: Could this be a result of bug 1652270?

Comment 3

[Jonathan Watt [:jwatt]]

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?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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 ?

Comment 5

[Emilio Cobos Álvarez (: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...

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1659758 - Null-check event target in nsDocShellTreeOwner::AddChromeListeners. r=smaug

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/062c7597ed2c
Null-check event target in nsDocShellTreeOwner::AddChromeListeners. r=smaug

Comment 9

[Emilio Cobos Álvarez (:emilio)]

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

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/062c7597ed2c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/57fc01dc4a01