1261718 - in nsSubDocumentFrame when getting the document from a view go through the view manager

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Timothy Nikkel (:tnikkel)]

The view is not guaranteed to have a frame (and won't during the early parts of the presshell's existence). It will have a view manager, and that will have a presshell during this time period.

Anytime a view has a frame it will also have a view manager and a pressshell so this is strictly better.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 2

[Mats Palmgren (inactive)]

Comment on attachment 8737647 [details] [diff] [review]
patch

I'd be surprised if the view doesn't have a frame here, for the current
call sites.  But sure, this is strictly better and a tiny bit faster too.

I think you can remove the 'vm' null-check, a view always points back
to its viewmanager for its entire lifetime, IIRC. (even when it's
detached)

Comment 3

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #2)
> I'd be surprised if the view doesn't have a frame here, for the current
> call sites.  But sure, this is strictly better and a tiny bit faster too.

We can reconstruct an iframe at any time while it is navigating to a new document, so I think it is quite possible for a view to not have a frame here.

Comment 4

[Mats Palmgren (inactive)]

Yes, but the views here are all owned by their respective frames, AFAIK.
So if the frame is destroyed so should its view which /should/ unhook it
from its siblings so you won't see that view in EndSwapDocShellsForViews.

I don't see anything that clears nsFrameLoader::mDetachedSubdocViews though,
in case the very first view in the chain got destroyed.  Hmm, we might
want to fix that just in case it's possible for navigation to occur
while being detached (i.e. between Begin[End]SwapDocShellsForViews).

I see that we call SetContainer(nullptr) recursively in DetachContainerRecurse,
but I don't know what that means for being able to navigate.

Perhaps we should add an assertion about it in ViewportFrame::DestroyFrom ?
(i.e. that its view != nsFrameLoader::mDetachedSubdocViews)
Then again, if it is, then it might be impossible to get at the associated
SubDocumentFrame's loader precisely because it is detached...

Comment 5

[Timothy Nikkel (:tnikkel)]

Hiding this bug so we can discuss this here instead of trying to move the discussion elsewhere. This bug and patch shouldn't be security issues I don't think.

(In reply to Mats Palmgren (:mats) from comment #4)
> Yes, but the views here are all owned by their respective frames, AFAIK.
> So if the frame is destroyed so should its view which /should/ unhook it
> from its siblings so you won't see that view in EndSwapDocShellsForViews.

The case I am thinking of is the root view, before the root frame has been constructed. There is a significant amount of time between those two events, I have verified this with a testcase.

We create the root view in nsDocumentViewer::MakeWindow

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#2388

before any frames have been constructed. And then later, in nsCSSFrameConstructor::ConstructRootFrame

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#2643

we set the root view as the view of the root frame we just constructed. I have verified with a testcase reconstructs an iframe every 150ms and then starts the iframe navigating to a new document that we do indeed have a root view without a frame for a significant amount of time.

> I don't see anything that clears nsFrameLoader::mDetachedSubdocViews though,
> in case the very first view in the chain got destroyed.  Hmm, we might
> want to fix that just in case it's possible for navigation to occur
> while being detached (i.e. between Begin[End]SwapDocShellsForViews).

Yes, I agree, see bug 1261175, comment 5. Planning to fix that too (perhaps by holding refpts to the corresponding view managers or documents or presshells instead of raw nsView pointers).

> I see that we call SetContainer(nullptr) recursively in
> DetachContainerRecurse,
> but I don't know what that means for being able to navigate.
> 
> Perhaps we should add an assertion about it in ViewportFrame::DestroyFrom ?
> (i.e. that its view != nsFrameLoader::mDetachedSubdocViews)
> Then again, if it is, then it might be impossible to get at the associated
> SubDocumentFrame's loader precisely because it is detached...

Good idea. We might be able to access it via GetFrameElement or via the docshell or via the subdocument map that nsIDocument manages. Hopefully we can just fix the potential problem though like I mentioned above.

Comment 6

[Daniel Veditz [:dveditz]]

Is there a specific security concern here, or is this more of a sec-audit cleanup? If the view didn't have a frame would it return something other than null? Because the null case was already handled.

Comment 7

[Mats Palmgren (inactive)]

(In reply to Timothy Nikkel (:tnikkel) from comment #5)
> We create the root view in nsDocumentViewer::MakeWindow [...]

Thanks for elaborating.  But if we have a root view with no root
frame yet, I don't see how we can have frames/views (or even shells)
for any sub-documents (so returning null from GetDocumentFromView
seems OK in that case, because there is nothing to do for
BeginSwapDocShellsForViews)

Perhaps you're worried about lingering pres stuff from a previous
navigation? but that stuff would be under a different root view, no?

Comment 8

[Timothy Nikkel (:tnikkel)]

(In reply to Daniel Veditz [:dveditz] from comment #6)
> Is there a specific security concern here, or is this more of a sec-audit
> cleanup? If the view didn't have a frame would it return something other
> than null? Because the null case was already handled.

I hid the bug only so that I could fully reply to Mats' comment (where I discuss actual security bugs). The pointers involved here should either be null or valid. The patch and problem here shouldn't be a security issue that I can tell.

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #7)
> Thanks for elaborating.  But if we have a root view with no root
> frame yet, I don't see how we can have frames/views (or even shells)
> for any sub-documents (so returning null from GetDocumentFromView
> seems OK in that case, because there is nothing to do for
> BeginSwapDocShellsForViews)

Yeah, doesn't seem like anything can go wrong.

> Perhaps you're worried about lingering pres stuff from a previous
> navigation? but that stuff would be under a different root view, no?

No, I don't think anything can go wrong. Just seemed silly to not get the doc if we do have access to it.

Comment 10

[Carsten Book [:Tomcat]]

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