1433669 - Crash in mozilla::PresShell::DoFlushPendingNotifications

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is
report bp-af952ffa-d978-4c73-bb6e-e3b900180127.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::PresShell::DoFlushPendingNotifications layout/base/PresShell.cpp:4158
1 xul.dll mozilla::PresShell::DoFlushPendingNotifications layout/base/PresShell.cpp:4089
2 xul.dll mozilla::ContentEventHandler::InitBasic dom/events/ContentEventHandler.cpp:258
3 xul.dll mozilla::ContentEventHandler::InitCommon dom/events/ContentEventHandler.cpp:338
4 xul.dll mozilla::ContentEventHandler::Init dom/events/ContentEventHandler.cpp:426
5 xul.dll mozilla::ContentEventHandler::OnQuerySelectedText dom/events/ContentEventHandler.cpp:1435
6 xul.dll mozilla::IMEContentObserver::UpdateSelectionCache dom/events/IMEContentObserver.cpp:1584
7 xul.dll mozilla::IMEContentObserver::IMENotificationSender::SendSelectionChange dom/events/IMEContentObserver.cpp:2039
8 xul.dll mozilla::IMEContentObserver::IMENotificationSender::Run dom/events/IMEContentObserver.cpp:1913
9 xul.dll nsRefreshDriver::Tick layout/base/nsRefreshDriver.cpp:1831

=============================================================

There are 48 crashes in nightly 60 starting with buildid 20180126035135 so after the patch for bug 1433056 landed.
:emilio, could you investigate please ?

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1433669: Remove dead function.

Review commit: https://reviewboard.mozilla.org/r/216188/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216188/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1433669: Flush the document instead of the shell in ContentEventHandler.

This allows us to maintain the pre-existing but not-asserted-before invariant of
not doing layout on documents in the BFCache.

The simpler fix here is something like:

  if (nsIDocument* doc = mPresShell->GetDocument()) {
    doc->FlushPendingNotifications();
  }

But referencing the document looks cleaner on most callsites I think. I can just
do the above if you prefer.

Review commit: https://reviewboard.mozilla.org/r/216190/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216190/

Comment 3

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

Comment on attachment 8946217 [details]
Bug 1433669: Remove dead function.

https://reviewboard.mozilla.org/r/216188/#review221974

Comment 4

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

Comment on attachment 8946218 [details]
Bug 1433669: Flush the document instead of the shell in ContentEventHandler.

https://reviewboard.mozilla.org/r/216190/#review221976

::: dom/events/ContentEventHandler.cpp:2668
(Diff revision 1)
>    rv = nsCopySupport::GetTransferableForSelection(
> -         mSelection, doc, getter_AddRefs(aEvent->mReply.mTransferable));
> +         mSelection, mDocument, getter_AddRefs(aEvent->mReply.mTransferable));

You're removing the check if the document is nullptr or not. So, mDocument might be nullptr here. However, nsCopySupport::GetTransferableForSelection() doesn't check it in release build. So, please keep checking it before here.
https://searchfox.org/mozilla-central/rev/11d0ff9f36465ce19b0c43d1ecc3025791eeb808/dom/base/nsCopySupport.cpp#124-127

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8946218 [details]
Bug 1433669: Flush the document instead of the shell in ContentEventHandler.

https://reviewboard.mozilla.org/r/216190/#review221984

::: dom/events/ContentEventHandler.cpp:2668
(Diff revision 1)
>    rv = nsCopySupport::GetTransferableForSelection(
> -         mSelection, doc, getter_AddRefs(aEvent->mReply.mTransferable));
> +         mSelection, mDocument, getter_AddRefs(aEvent->mReply.mTransferable));

It cannot be null because `Init` has succeeded, unless I'm missing something.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44b8352bbee0
Remove dead function. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/4a354c4ded06
Flush the document instead of the shell in ContentEventHandler. r=masayuki

Comment 7

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

Comment on attachment 8946218 [details]
Bug 1433669: Flush the document instead of the shell in ContentEventHandler.

https://reviewboard.mozilla.org/r/216190/#review221984

> It cannot be null because `Init` has succeeded, unless I'm missing something.

Ah, got it. Thanks.

Comment 8

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/44b8352bbee0
https://hg.mozilla.org/mozilla-central/rev/4a354c4ded06