nsSubDocumentFrame uses content to get docshell

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: stechz, Assigned: stechz)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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. :)
(Assignee)

Comment 1

6 years ago
Created attachment 533435 [details] [diff] [review]
nsSubDocumentFrame uses content to get docshell
Attachment #533435 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Assignee: nobody → ben
>+      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.)
(Assignee)

Comment 3

6 years ago
> 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.
> 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!
(Assignee)

Comment 5

6 years ago
> 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.
That sounds fine to me.
(Assignee)

Comment 7

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

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

Comment 8

6 years ago
Created attachment 533681 [details] [diff] [review]
nsSubDocumentFrame uses content to get docshell
Attachment #533681 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Attachment #533435 - Attachment is obsolete: true
Attachment #533435 - Flags: review?(bzbarsky)
Comment on attachment 533681 [details] [diff] [review]
nsSubDocumentFrame uses content to get docshell

r=me
Attachment #533681 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 10

6 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/c7113f78446b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.