1551241 - Crash in [@ mozilla::PresShell::SetIsUnderHiddenEmbedderElement]

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Calixte Denizet (:calixte)]

This bug is for crash report bp-a6285ce6-2d80-4114-bfc3-d51320190513.

Top 10 frames of crashing thread:

0 xul.dll mozilla::PresShell::SetIsUnderHiddenEmbedderElement layout/base/PresShell.cpp:10926
1 xul.dll void nsSubDocumentFrame::DidSetComputedStyle layout/generic/nsSubDocumentFrame.cpp:942
2 xul.dll bool mozilla::RestyleManager::ProcessPostTraversal layout/base/RestyleManager.cpp:2846
3 xul.dll bool mozilla::RestyleManager::ProcessPostTraversal layout/base/RestyleManager.cpp:2892
4 xul.dll bool mozilla::RestyleManager::ProcessPostTraversal layout/base/RestyleManager.cpp:2892
5 xul.dll bool mozilla::RestyleManager::ProcessPostTraversal layout/base/RestyleManager.cpp:2892
6 xul.dll bool mozilla::RestyleManager::ProcessPostTraversal layout/base/RestyleManager.cpp:2892
7 xul.dll void mozilla::RestyleManager::DoProcessPendingRestyles layout/base/RestyleManager.cpp:3093
8 xul.dll mozilla::PresShell::DoFlushPendingNotifications layout/base/PresShell.cpp:4179
9 xul.dll mozilla::dom::Document::FlushPendingNotifications dom/base/Document.cpp:7343

There is 1 crash in nightly 68 with buildid 20190513082256. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1518919.

[1] https://hg.mozilla.org/mozilla-central/rev?node=d77d76d37d4d

Comment 1

[Jonathan Watt [:jwatt]]

I have to go AFK for an hour or two, but I'll look at this once I get back.

Comment 2

[Jonathan Watt [:jwatt]]

CC'ing nika since from my conversation with her on IRC this sounds unexpected.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

I supposed that the GetEmbedderElement returns a valid Element* if the child BrowsingContext exists, but it seems not. :/
I am going to add a null check there.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1551241 - Add a check for the case where the embedder element is nullptr. r?jwatt

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Oops, I did miss jwatt's comment in comment 2.
I will find out the IRC log.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Assigning to myself tentatively, nika might look at this though.

Comment 7

[Jonathan Watt [:jwatt]]

The summary from IRC was that a null check shouldn't be necessary, but nika thought there might be an issue with dynamic frame removal or some bfcache issue that farre has been looking into. Dynamic frame removal would seem like a weird source of such an issue to me, since nulling out BrowsingContext::mEmbedderElement but not removing the BrowsingContext from it's parent BrowsingContext object (all in the same process) would seem weird. But yes, hopefully nika can comment here about what's most likely. If we are going to add a null check, it would be good to add a comment in the code saying that it shouldn't be necessary but explaining why it currently is.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6ae14913216
Add a check for the case where the embedder element is nullptr. r=jwatt

Comment 9

[Nika Layzell [:nika] (ni? for response)]

(In reply to Jonathan Watt [:jwatt] from comment #7)

The summary from IRC was that a null check shouldn't be necessary, but nika thought there might be an issue with dynamic frame removal or some bfcache issue that farre has been looking into. Dynamic frame removal would seem like a weird source of such an issue to me, since nulling out BrowsingContext::mEmbedderElement but not removing the BrowsingContext from it's parent BrowsingContext object (all in the same process) would seem weird. But yes, hopefully nika can comment here about what's most likely. If we are going to add a null check, it would be good to add a comment in the code saying that it shouldn't be necessary but explaining why it currently is.

In nsFrameLoader::StartDestroy we call SetOwnerContent(nullptr) on the nsFrameLoader: https://searchfox.org/mozilla-central/rev/116bd975c30746ddefc3d20e6947d1871469354f/dom/base/nsFrameLoader.cpp#1828
This then gets the BrowsingContext and sets its EmbedderElement to nullptr as well. https://searchfox.org/mozilla-central/rev/116bd975c30746ddefc3d20e6947d1871469354f/dom/base/nsFrameLoader.cpp#1997-1999

:jwatt You could try to add a null guard there, to prevent setting a BrowsingContext's EmbedderElement to nullptr.

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/f6ae14913216

Comment 11

[Jonathan Watt [:jwatt]]

(In reply to :Nika Layzell (ni? for response) from comment #9)

:jwatt You could try to add a null guard there, to prevent setting a BrowsingContext's EmbedderElement to nullptr.

I did try that but it causes us to leak documents. I left the needinfo with the intention of circling back and digging into this further to see if we could do something else (delay the nulling out maybe?), but I guess I should just admit that that's not going to get to the top of my todo list.