Closed Bug 1551241 Opened 4 months ago Closed 4 months ago

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

Categories

(Core :: Layout, defect, P1, critical)

68 Branch
x86
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- unaffected
firefox67 --- unaffected
firefox67.0.1 --- unaffected
firefox68 --- fixed

People

(Reporter: calixte, Assigned: hiro)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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

Flags: needinfo?(hikezoe)

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

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

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.

Flags: needinfo?(hikezoe)

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

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

Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Priority: -- → P1

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.

Flags: needinfo?(nika)
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f6ae14913216
Add a check for the case where the embedder element is nullptr. r=jwatt

(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.

Flags: needinfo?(nika) → needinfo?(jwatt)
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

(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.

Flags: needinfo?(jwatt)
You need to log in before you can comment on or make changes to this bug.