Closed Bug 1435566 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Layout, defect)

60 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- wontfix
firefox59 --- fixed
firefox60 --- fixed

People

(Reporter: tsmith, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [adv-main59+])

Attachments

(4 files)

Attached file 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)
Flags: in-testsuite?
this sounds rather bad. I'll try to find time to investigate.
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.
Component: DOM → Layout
Flags: needinfo?(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.
Hmm, the NAC span is already unbound from the tree before the frames get destroyed, so this is probably an editor issue...
But why display: contents is needed to trigger the issue?
Group: dom-core-security → layout-core-security
(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.
Attached file 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.
(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...
Now gotta see what posted the reconstruct for the <span>, doing that is bogus afaict...
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.
Flags: needinfo?(emilio) → needinfo?(bzbarsky)
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?
Attachment #8950675 - Flags: review?(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.
Flags: needinfo?(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?
Flags: needinfo?(emilio)
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"...
(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?
Flags: needinfo?(emilio)
> Happy to do so, you know which one is the relevant spec?

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

r=me
Attachment #8950675 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/3219cd61e87b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Group: layout-core-security → core-security-release
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
Attachment #8950675 - Flags: approval-mozilla-release?
Attachment #8950675 - Flags: approval-mozilla-beta?
Can this get a security rating?
Flags: needinfo?(dveditz)
Doesn't seem like it fits the criteria for dot release consideration. Taking for 59b12, however.
Assignee: nobody → emilio
Attachment #8950675 - Flags: approval-mozilla-release?
Attachment #8950675 - Flags: approval-mozilla-release-
Attachment #8950675 - Flags: approval-mozilla-beta?
Attachment #8950675 - Flags: approval-mozilla-beta+
Whiteboard: [adv-main59+]
Group: core-security-release
Flags: needinfo?(dveditz)
You need to log in before you can comment on or make changes to this bug.