658076 - nsSubDocumentFrame uses content to get docshell

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Benjamin Stover (:stechz)]

Using content to get docshell is fine for local browsers, but this does not hold true for remote browsers. Since I don't know how to get the docshell I've just returned early. If someone knows how to get the docshell otherwise, I'd be happy to do so. :)

Comment 1

[Benjamin Stover (:stechz)]

Attachment: nsSubDocumentFrame uses content to get docshell

Comment 2

[Boris Zbarsky [:bzbarsky]]

>+      while (true) {}

What's the point of this change?

You can't get a docshell pointer in the remote browser case, because the docshell is in the other process.

The code without this change will get null for docShellAsItem and return early.  So this change is not actually changing behavior at all as far as I can tell... (well, except for the hangs introduced by the while (true) bit on other codepaths).

What are you actually trying to do here?

(On a side note, it does look like we don't communicate the content-primary vs content-targetable changes down to the content process.... or do we do that somewhere else?  Or do we not have such a distinction at all here?  It might not matter if you don't allow sidebars.)

Comment 3

[Benjamin Stover (:stechz)]

> What's the point of this change?

Sorry, leftover debugging stuff!

> You can't get a docshell pointer in the remote browser case, because the
> docshell is in the other process.

Oh, I thought it was trying to get the parent docshell of the content frame, which I thought would be in the main process.

> What are you actually trying to do here?

Remove a warning.

> (On a side note, it does look like we don't communicate the content-primary
> vs content-targetable changes down to the content process.... or do we do
> that somewhere else?  Or do we not have such a distinction at all here?  It
> might not matter if you don't allow sidebars.)

I believe we don't do that right now. Perhaps we should file a separate bug for Firefox's benefit.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Oh, I thought it was trying to get the parent docshell of the content frame

Ah, no.  It's trying to get the child docshell.

> Remove a warning.

We should consider just removing the warning, then...  On the other hand, in this case the warning is just showing the existence of a bug: in the ipc case we're not handling the primary-vs-targetable distinction correctly.  So the warning is doing its job!

Comment 5

[Benjamin Stover (:stechz)]

> We should consider just removing the warning, then...  On the other hand, in
> this case the warning is just showing the existence of a bug: in the ipc
> case we're not handling the primary-vs-targetable distinction correctly.  So
> the warning is doing its job!

It's a good point. I like the warning in GetDocShell because it points to broken things, and I suppose an early return isn't really fixing anything.

Let me propose this: leave the warning for GetDocShell in, add a TODO with a bug # and an early return.

Comment 6

[Boris Zbarsky [:bzbarsky]]

That sounds fine to me.

Comment 7

[Benjamin Stover (:stechz)]

Also, is this code doing almost exactly the same thing?

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#699

Comment 8

[Benjamin Stover (:stechz)]

Attachment: nsSubDocumentFrame uses content to get docshell

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 533681 [details] [diff] [review]
nsSubDocumentFrame uses content to get docshell

r=me

Comment 10

[Benjamin Stover (:stechz)]

Pushed http://hg.mozilla.org/mozilla-central/rev/c7113f78446b