1555645 - Crash in [@ mozilla::PresShell::EventHandler::MaybeFlushThrottledStyles]

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P2)

Description

[[:philipp]]

This bug is for crash report bp-485be257-dcbc-4ca5-aaef-7be9c0190530.

Top 10 frames of crashing thread:

0 xul.dll class nsIFrame* mozilla::PresShell::EventHandler::MaybeFlushThrottledStyles layout/base/PresShell.cpp:7334
1 xul.dll nsresult mozilla::PresShell::EventHandler::HandleEventUsingCoordinates layout/base/PresShell.cpp:6672
2 xul.dll mozilla::PresShell::EventHandler::HandleEvent layout/base/PresShell.cpp:6613
3 xul.dll bool mozilla::PresShell::EventHandler::MaybeHandleEventWithAnotherPresShell layout/base/PresShell.cpp:7230
4 xul.dll mozilla::PresShell::EventHandler::HandleEvent layout/base/PresShell.cpp:6600
5 xul.dll nsViewManager::DispatchEvent view/nsViewManager.cpp:752
6 xul.dll nsEventStatus nsView::HandleEvent view/nsView.cpp:1072
7 xul.dll mozilla::widget::PuppetWidget::DispatchEvent widget/PuppetWidget.cpp:390
8 xul.dll mozilla::layers::APZCCallbackHelper::DispatchWidgetEvent gfx/layers/apz/util/APZCCallbackHelper.cpp:544
9 xul.dll void mozilla::dom::BrowserChild::HandleRealMouseButtonEvent dom/ipc/BrowserChild.cpp:1663

this crash signature is showing up cross-platform since firefox 67 (in low volume though), possibly related to the changes from bug 1466208.

Comment 1

[Edgar Chen [:edgar]]

It seems like a regression of bug 1466208, could you take a look, Masayuki? Thanks.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Looks like that this is not a regression of it. We have crash reports around there with same reason even before that:
https://crash-stats.mozilla.org/report/index/1d6da93c-2f09-4539-885d-245990190603
https://hg.mozilla.org/releases/mozilla-esr60/annotate/79253cb82ff8d415746056b6ee6a01a0564b7c50/layout/base/PresShell.cpp#l6551
Anyway, looks like that GetRootPresShell() may return nullptr.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1555645 - Make PresShell::EventHandler::MaybeFlushThrottledStyles() stop handling it when the PresShell does not have root PreShell nor Document

The crash reports just tell us that the crash occur due to referring around
address 0 in PresShell::EventHandler::MaybeFlushThrottledStyles().
Therefore, I'm not sure which is the actual reason of the crashes though,
this patch makes it null-check root PresShell and its Document before
accessing the latter.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9550b1e1dfb1
Make PresShell::EventHandler::MaybeFlushThrottledStyles() stop handling it when the PresShell does not have root PreShell nor Document r=smaug

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9069830 [details]
Bug 1555645 - Make PresShell::EventHandler::MaybeFlushThrottledStyles() stop handling it when the PresShell does not have root PreShell nor Document

Beta/Release Uplift Approval Request

• User impact if declined: May meet this crash.
• 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:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just adding nullptr checks.
• String changes made/needed: none.

Comment 6

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/9550b1e1dfb1

Comment 7

[Julien Cristau [:jcristau]]

Comment on attachment 9069830 [details]
Bug 1555645 - Make PresShell::EventHandler::MaybeFlushThrottledStyles() stop handling it when the PresShell does not have root PreShell nor Document

nullptr checks, approved for 68.0b9

Comment 8

[Razvan Maries]

https://hg.mozilla.org/releases/mozilla-beta/rev/16500a22225b

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Reopening since the crash still persists. e.g. bp-5c551a82-1ea7-4c5b-ad66-d9d3b0200423

I haven't audited all crash reports, but as far as I can tell the call stack is including MaybeHandleEventWithAnotherPresShell which means the crash happened inside a nested document. And the crash happened at the code around we tried to get the root pres shell, it may be possible that the root pres shell is an ancestor of the initial target pres shell, I am quite unsure though.

Anyways, my question is "do we really need to flush throttled styles for the root pres shell?". My answer is "no". Given that we've already called MaybeFlushPendingNotifications just before we call MaybeFlushThrottledStyles, it would be sufficient.

As a side note about MaybeFlushThrottledStyles, emilo has a plan to drop the MaybeFlushThrottledStyles call.

Comment 10

[Julien Cristau [:jcristau]]

Can we track the remaining issues in a new bug rather than reopen this?

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Filed bug 1633019.