1435566 - Assertion failure: IsInUncomposedDoc() || IsInShadowTree() (This will end badly!), at /src/dom/base/nsIContentInlines.h:35

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Marking as s-s for now if this is not a security issue please open the bug.

STR:
1) launch browser
2) open testcase in a new tab
3) once testcase is loaded close tab

Note: If prefs are set to allow pages to call window.close() testcase will work automatically. 

Assertion failure: IsInUncomposedDoc() || IsInShadowTree() (This will end badly!), at /src/dom/base/nsIContentInlines.h:35

#0 nsIContent::SetPrimaryFrame(nsIFrame*) /src/dom/base/nsIContentInlines.h:35:3
#1 nsFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /src/layout/generic/nsFrame.cpp:836:15
#2 nsContainerFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /src/layout/generic/nsContainerFrame.cpp:303:22
#3 nsFrameList::DestroyFramesFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /src/layout/generic/nsFrameList.cpp:59:12
#4 nsContainerFrame::DestroyAbsoluteFrames(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /src/layout/generic/nsContainerFrame.cpp:193:35
#5 nsContainerFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /src/layout/generic/nsContainerFrame.cpp:227:3
#6 nsCanvasFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /src/layout/generic/nsCanvasFrame.cpp:164:21
#7 nsFrameList::DestroyFramesFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /src/layout/generic/nsFrameList.cpp:59:12
#8 nsContainerFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /src/layout/generic/nsContainerFrame.cpp:230:11
#9 nsFrameList::DestroyFramesFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /src/layout/generic/nsFrameList.cpp:59:12
#10 nsContainerFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /src/layout/generic/nsContainerFrame.cpp:230:11
#11 nsIFrame::Destroy() /src/layout/generic/nsIFrame.h:687:5
#12 nsFrameManager::Destroy() /src/layout/base/nsFrameManager.cpp:118:17
#13 mozilla::PresShell::Destroy() /src/layout/base/PresShell.cpp:1391:22
#14 nsDocumentViewer::DestroyPresShell() /src/layout/base/nsDocumentViewer.cpp:4641:15
#15 nsDocumentViewer::Destroy() /src/layout/base/nsDocumentViewer.cpp:1781:5
#16 nsDocShell::Destroy() /src/docshell/base/nsDocShell.cpp:5498:21
#17 non-virtual thunk to nsDocShell::Destroy() /src/docshell/base/nsDocShell.cpp
#18 nsWebBrowser::SetDocShell(nsIDocShell*) /src/toolkit/components/browser/nsWebBrowser.cpp:1712:23
#19 nsWebBrowser::InternalDestroy() /src/toolkit/components/browser/nsWebBrowser.cpp:95:3
#20 nsWebBrowser::Destroy() /src/toolkit/components/browser/nsWebBrowser.cpp:1306:3
#21 non-virtual thunk to nsWebBrowser::Destroy() /src/toolkit/components/browser/nsWebBrowser.cpp
#22 mozilla::dom::TabChild::DestroyWindow() /src/dom/ipc/TabChild.cpp:1075:21
#23 mozilla::dom::TabChild::RecvDestroy() /src/dom/ipc/TabChild.cpp:2571:3
#24 mozilla::dom::PBrowserChild::OnMessageReceived(IPC::Message const&) /src/obj-firefox/ipc/ipdl/PBrowserChild.cpp:4575:20
#25 mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) /src/obj-firefox/ipc/ipdl/PContentChild.cpp:5023:28
#26 mozilla::dom::ContentChild::OnMessageReceived(IPC::Message const&) /src/dom/ipc/ContentChild.cpp:3725:25
#27 mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /src/ipc/glue/MessageChannel.cpp:2110:25
#28 mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /src/ipc/glue/MessageChannel.cpp:2040:17
#29 mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /src/ipc/glue/MessageChannel.cpp:1886:5
#30 mozilla::ipc::MessageChannel::MessageTask::Run() /src/ipc/glue/MessageChannel.cpp:1919:15
#31 mozilla::SchedulerGroup::Runnable::Run() /src/xpcom/threads/SchedulerGroup.cpp:395:25
#32 nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1040:14
#33 NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:517:10
#34 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21
#35 MessageLoop::RunInternal() /src/ipc/chromium/src/base/message_loop.cc:326:10
#36 MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299:3
#37 nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:157:27
#38 XRE_RunAppShell() /src/toolkit/xre/nsEmbedFunctions.cpp:873:22
#39 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:269:9
#40 MessageLoop::RunInternal() /src/ipc/chromium/src/base/message_loop.cc:326:10
#41 MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299:3
#42 XRE_InitChildProcess(int, char**, XREChildData const*) /src/toolkit/xre/nsEmbedFunctions.cpp:699:34
#43 content_process_main(mozilla::Bootstrap*, int, char**) /src/browser/app/../../ipc/contentproc/plugin-container.cpp:63:30
#44 main /src/browser/app/nsBrowserApp.cpp:280:18
#45 __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
#46 _start (firefox+0x423444)

Comment 1

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

this sounds rather bad. I'll try to find time to investigate.

Comment 2

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

This looks a lot more like a layout issue, related to display: contents handling, combined with native anonymous content coming from editor.

Weird thing is that nsFrame::DestroyFrom  seems to be called on a frame which mContent's primary frame has been cleared already, or at least its SubtreeRoot() == this in nsIContent::SetPrimaryFrame.


mats and emilio should know display: contents handling better.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Right, so this happens because we destroy the frames of an element after unbinding the element from the document, which is wrong. We have aPostDestroyData for handling this properly for NAC, but apparently this one manages to get unbound early... I'm debugging right now.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Hmm, the NAC span is already unbound from the tree before the frames get destroyed, so this is probably an editor issue...

Comment 5

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

But why display: contents is needed to trigger the issue?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] from comment #5)
> But why display: contents is needed to trigger the issue?

That's a good question... I'm trying to figure that out.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: UnbindFromTree stack.

At that point the primary frame of the anon node is still a block frame... The element getting unbound is the <body>.

I suspect the ContentRemoved notification for the <body> in the frame ctor isn't taking care of dropping the native anonymous frames.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> I suspect the ContentRemoved notification for the <body> in the frame ctor
> isn't taking care of dropping the native anonymous frames.

Well, this is exactly what happens, and the reason why it happens is unclear to me still, since there's no way a NAC element got frames out of something that doesn't have frames like the display: contents node...

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Stack of the NAC node getting frames.

Now gotta see what posted the reconstruct for the <span>, doing that is bogus afaict...

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Well, so we kind-of already whitelist this: https://searchfox.org/mozilla-central/rev/9d47b5fb14152865ad1375eb3ee2e571d81ecdb9/editor/libeditor/HTMLAnonymousNodeEditor.cpp#249-259

But in this case the node we're appending NAC to doesn't even have a frame (has display: contents), so this is completely bogus AFAICT, and the editor ends up lost. Is a display: contents node even supposed to be a valid editor root?

I'm not sure what's supposed to be the right fix for this.

I can hackily patch this so that NAC roots whose parent doesn't have a frame just don't create frames either... But that's really kind-of a hack :(

I can also just tear down manual NAC looking at the property on the frame constructor... That may be (kind-of?) saner...

Boris, wdyt of a solution like that? I can't think of anything better right now, but more ideas welcome, of course.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Make sure to drop manual NAC from display: contents nodes.

There's the question of what happens if the element gets reframed... Should we
add a "Flattened tree descendants + manual NAC" iterator and use that
consistently on frame construction? That'd be kind of annoying.

That being said, that is a problem that already seems to exist with the current
editor code, and nobody has an issue with that, so I assume the editor already
deals with it, even if it's recreating the NAC?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Ah, this editor hackery coming back to bite us.  :(  I really wish I had some idea of how it should work too.

In general, I think we should do the following:

1)  Specs should sort out how contenteditable interacts with display:contents;
    need spec issues here.  In the meantime, we should consider not attaching
    a non-document-level editor to a display:contents node or something.  Maybe.
    Changing editor attachment on display changes is weird too...
2)  For document-level designMode, as here, editor should probably use
    document-level anonymous content instead of the thing it's doing right
    now.  Followup bug for that?  I'd hope that we end up with more sanity that way.

I'm not sure this last works for things like resizers, actually; even if the designMode is document-level the resizers get attached to a specific element right now.  We could maybe still make them doc-level and reuse the existing positioning code for the "parent is not positioned" case to just place them in the right spot...  Maybe we can even do that for contenteditable?  Someone would need to sit down and actually look through how this anon content ends up getting used.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8950675 [details] [diff] [review]
Make sure to drop manual NAC from display: contents nodes.

That would iterate ::before and ::after too.  Is that OK?

Comment 14

[Boris Zbarsky [:bzbarsky]]

So the key part is that this NAC's frame gets parented outside the <body>, right?

I'm kinda liking the "don't create a frame for the NAC if its parent has no frame"...

Comment 15

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #13)
> Comment on attachment 8950675 [details] [diff] [review]
> Make sure to drop manual NAC from display: contents nodes.
> 
> That would iterate ::before and ::after too.  Is that OK?

It should. Indeed that's handled a few lines above: https://searchfox.org/mozilla-central/rev/d03ad8843e3bf2e856126bc53b0475c595e5183b/layout/base/nsCSSFrameConstructor.cpp#8568

But I suspect with this patch we can just remove that whole condition too.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #14)
> So the key part is that this NAC's frame gets parented outside the <body>,
> right?
> 
> I'm kinda liking the "don't create a frame for the NAC if its parent has no
> frame"...

Given the above, we might as well take this patch and avoid both issues.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #12)
> Ah, this editor hackery coming back to bite us.  :(  I really wish I had
> some idea of how it should work too.
> 
> In general, I think we should do the following:
> 
> 1)  Specs should sort out how contenteditable interacts with
> display:contents;
>     need spec issues here.  In the meantime, we should consider not attaching
>     a non-document-level editor to a display:contents node or something. 
> Maybe.
>     Changing editor attachment on display changes is weird too...

Happy to do so, you know which one is the relevant spec?

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Happy to do so, you know which one is the relevant spec?

HTML, probably....

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8950675 [details] [diff] [review]
Make sure to drop manual NAC from display: contents nodes.

r=me

Comment 18

[Emilio Cobos Álvarez (:emilio)]

remote: View your change here:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3219cd61e87b4f3003079154c437efa87080a917

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3219cd61e87b

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8950675 [details] [diff] [review]
Make sure to drop manual NAC from display: contents nodes.

So after Tyson asked whether this bug could be opened or not, I checked with bz and we decided to keep it hidden for now.

I think it's not exploitable since the DOM node that remains detached shouldn't be reachable anymore, given it's NAC. But it's very hard to audit for it in depth, so uplifting it is less risky.

Approval Request Comment
[Feature/Bug causing the regression]: Probably bug 1386110
[User impact if declined]: potential security issue.
[Is this code covered by automated tests?]: no (haven't pushed the crashtest)
[Has the fix been verified in Nightly?]: no (we only managed to fire an assertion so far)
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: just a one liner that makes us iterate over some more nodes that we didn't use to account for.
[String changes made/needed]: none

Comment 21

[Julien Cristau [:jcristau]]

Can this get a security rating?

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Doesn't seem like it fits the criteria for dot release consideration. Taking for 59b12, however.

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/70bf6bd7b101