Last Comment Bug 658076 - nsSubDocumentFrame uses content to get docshell
: nsSubDocumentFrame uses content to get docshell
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Benjamin Stover (:stechz)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-18 14:54 PDT by Benjamin Stover (:stechz)
Modified: 2011-06-09 10:02 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
nsSubDocumentFrame uses content to get docshell (1.85 KB, patch)
2011-05-18 14:55 PDT, Benjamin Stover (:stechz)
no flags Details | Diff | Splinter Review
nsSubDocumentFrame uses content to get docshell (1.15 KB, patch)
2011-05-19 09:52 PDT, Benjamin Stover (:stechz)
bzbarsky: review+
Details | Diff | Splinter Review

Description Benjamin Stover (:stechz) 2011-05-18 14:54:59 PDT
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) 2011-05-18 14:55:40 PDT
Created attachment 533435 [details] [diff] [review]
nsSubDocumentFrame uses content to get docshell
Comment 2 Boris Zbarsky [:bz] (TPAC) 2011-05-18 21:00:39 PDT
>+      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) 2011-05-19 09:17:53 PDT
> 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 [:bz] (TPAC) 2011-05-19 09:29:30 PDT
> 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) 2011-05-19 09:33:14 PDT
> 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 [:bz] (TPAC) 2011-05-19 09:34:31 PDT
That sounds fine to me.
Comment 7 Benjamin Stover (:stechz) 2011-05-19 09:35:42 PDT
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) 2011-05-19 09:52:23 PDT
Created attachment 533681 [details] [diff] [review]
nsSubDocumentFrame uses content to get docshell
Comment 9 Boris Zbarsky [:bz] (TPAC) 2011-05-19 13:08:42 PDT
Comment on attachment 533681 [details] [diff] [review]
nsSubDocumentFrame uses content to get docshell

r=me
Comment 10 Benjamin Stover (:stechz) 2011-06-09 10:02:27 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/c7113f78446b

Note You need to log in before you can comment on or make changes to this bug.