Closed Bug 771273 Opened 12 years ago Closed 12 years ago

Make nsDocShell::GetSameTypeParent and friends respect <iframe mozbrowser> boundaries

Categories

(Core :: DOM: Navigation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: justin.lebar+bug, Unassigned)

References

Details

Attachments

(5 files, 1 obsolete file)

In-process <iframe mozbrowser> is being used more and more, in places I didn't expect.  See for example bug 762993.

If we're loading arbitrary code inside in-process mozbrowser, we need to lock it down properly.  First step is window.open(_top/parent).
Blocks: browser-api
Actually, now that I've thought about this more, I'm starting to think we should just fix nsDocShell::GetSameTypeParent and nsDocShell::GetSameTypeRoot to respect <iframe mozbrowser> boundaries.  That's much better than playing whack-a-mole, as I've done with the other in-process bugs.

I'd bee hoping not to do this, but perhaps the time has come.  I guess the good news is, we can be aggressive and switch almost everything to respecting mozbrowser boundaries.  If things break, it'll only be where mozbrowser is used, and the change will make us much less broken than we currently are!
Component: DOM: Mozilla Extensions → Document Navigation
Summary: BrowserAPI: Handle _top and _parent targets correctly in <iframe mozbrowser> → Make nsDocShell::GetSameTypeParent and friends respect <iframe mozbrowser> boundaries
Depends on: 764718
Blocks: 769182
I'm not sure that this is entirely correct -- there's a lot of code to audit, and I'm not an expert -- but it's unlikely to be worse than what we currently have.

nsGlobalWindow basically ignores this change and keeps doing what it has done.  That may not be entirely correct, but it's definitely necessary in some places.
Attachment #639924 - Flags: review?(bzbarsky)
There are a lot more things we could test here -- without looking at the code, _parent and window.content come to mind.  But this is a start...
Attachment #639926 - Flags: review?(bzbarsky)
Depends on: 772076
Comment on attachment 639924 [details] [diff] [review]
Part 1: Make nsDocShell::GetSameTypeParent and friends respect <iframe mozbrowser> boundaries.

>+++ b/docshell/base/nsDocShell.cpp
> nsDocShell::GetSameTypeParent(nsIDocShellTreeItem ** aParent)

Can you make some of the nsGlobalWindow stuff call this now?

>+nsDocShell::GetParentIgnoreBrowserFrame(nsIDocShell** aParent)

This doesn't make it clear that the parent needs to be same-type...  I'm not sure how to indicate that without a ridiculous method name.  :(

> nsDocShell::IsFrame()
>+    return !mIsBrowserFrame && parent;

If mIsBrowserFrame, then parent will be null, no?  So can't this just |return parent;|?

>+++ b/dom/base/nsGlobalWindow.cpp
> nsGlobalWindow::GetContent(nsIDOMWindow** aContent)
>-    // If we're called by non-chrome code, make sure we don't return
>-    // the primary content window if the calling tab is hidden. In
>-    // such a case we return the same-type root in the hidden tab,
>-    // which is "good enough", for now.
....
>+    // If we're called by non-chrome code, this is the same as window.top().

That's not what the old code did.  In particular, window.content in a non-chrome sidebar was decidedly not the same as window.top...  Not that we have test coverage for that gunk.  :(

r- until we get this sorted out.

> nsGlobalWindow::GetRealFrameElement(nsIDOMElement** aFrameElement)
>     // We're at a chrome boundary, don't expose the chrome iframe
>     // element to content code.

See, I'm not clear why the same considerations don't apply to mozbrowser... but anyway.  This is certainly at least preserving behavior.
Attachment #639924 - Flags: review?(bzbarsky) → review-
Comment on attachment 639925 [details] [diff] [review]
Part 2: Make in-process <iframe mozbrowser>'s have their own shistory objects.

r=me
Attachment #639925 - Flags: review?(bzbarsky) → review+
Comment on attachment 639926 [details] [diff] [review]
Part 3: Test that window.open(..., 'top') works properly for in-process <iframe mozbrowser>.

r=me
Attachment #639926 - Flags: review?(bzbarsky) → review+
Comment on attachment 639927 [details] [diff] [review]
Part 4: Enable BackForward test for in-process <iframe mozbrowser>, now that it works.

r=me
Attachment #639927 - Flags: review?(bzbarsky) → review+
> See, I'm not clear why the same considerations don't apply to mozbrowser...

What considerations, exactly?  You mean, why does GetRealTop exist?  I haven't looked too hard, but there's at least some widget-y code which needs to find chrome windows.

> nsDocShell::GetSameTypeParent(nsIDocShellTreeItem ** aParent)
>
> Can you make some of the nsGlobalWindow stuff call this now?

You mean, like nsGlobalWindow::GetScriptableParent?  It's kind of awkward; what we really need is nsIDocShell::GetSameTypeParent2 which does /not/ respect mozbrowser boundaries, so we can implement GetRealParent in terms of that.  But I don't think GetScriptableParent2 has a lot of other potential uses.

>>+nsDocShell::GetParentIgnoreBrowserFrame(nsIDocShell** aParent)
> 
> This doesn't make it clear that the parent needs to be same-type...  I'm not sure how to indicate 
> that without a ridiculous method name.  :(

My thoughts exactly.  I'll try to think up a better name, but I'm not hopeful.
Okay, I /think/ I see...

If the window is "invisible" (background tab?), then we /do/ return window.top for the sidebar, right?  That's totally bizarre.

Can I tell whether the window is a sidebar?  I'm not having good luck searching for that.
> You mean, why does GetRealTop exist? 

No, why GetRealChromeElement exists.


> If the window is "invisible" (background tab?), then we /do/ return window.top for the 
> sidebar

Well, the sidebar is never really invisible...

Why not just keep the existing visibility check?
> No, why GetRealChromeElement exists.

Oh, GetRealFrameElement.  Yes, we can nix that.  Cool.

> Well, the sidebar is never really invisible...

I give up.  What's a sidebar?

> Why not just keep the existing visibility check?

AIUI, the existing code says:

  1) If we're invisible, window.content returns SameTypeRootTreeItem (which respects mozbrowser)
  2) Otherwise, window.content returns the tree owner's primary content shell (which doesn't respect mozbrowser).

The problem with keeping the visibility check is that, if we're visible but not a sidebar, we don't want to return the primary content shell.  I think we want to change the predicate in (1) to "If we're invisible or not a sidebar", but I don't know how to do that.

Or maybe I'm missing something here, because I really have no idea what this sidebar business is about.
> I give up.  What's a sidebar?

In Firefox, try View > Sidebar > Bookmarks?

But there are extensions that put other things in there too.

I see now what your problem is with the code as is is.  What's the equivalent of "primary content shell" in the mozbrowser world, assuming there is one?
> What's the equivalent of "primary content shell" in the mozbrowser world, assuming there is one?

I guess it depends on your perspective.  If you're inside a mozbrowser, the primary content shell should be the same as window.top, right?  You can't see outside the mozbrowser.  If you're outside mozbrowser, then the primary content shell is the same as it currently is.

I guess that answers my question about how to implement this, doesn't it?
Sounds plausible.  ;)
>> No, why GetRealChromeElement exists.
> 
> Oh, GetRealFrameElement.  Yes, we can nix that.  Cool.

If it's OK with you, I'd like to handle this in a follow-up.  I have enough trouble sticking patches in the tree these days, and I don't need to further complicate this landing...
Comment on attachment 641904 [details] [diff] [review]
Part 1, v2: Make nsDocShell::GetSameTypeParent and friends respect <iframe mozbrowser> boundaries.

Interdiff part 1 --> part2: is

$ interdiff <(curl -L https://bug771273.bugzilla.mozilla.org/attachment.cgi?id=641904) <(curl -L https://bug771273.bugzilla.mozilla.org/attachment.cgi?id=639924)
Attachment #639924 - Attachment is obsolete: true
> If it's OK with you, I'd like to handle this in a follow-up. 

Worksforme.
Blocks: 773675
Comment on attachment 641904 [details] [diff] [review]
Part 1, v2: Make nsDocShell::GetSameTypeParent and friends respect <iframe mozbrowser> boundaries.

r=me
Attachment #641904 - Flags: review?(bzbarsky) → review+
Comment on attachment 641906 [details] [diff] [review]
Part 5: Add test for window.content inside <iframe mozbrowser>.

r=me
Attachment #641906 - Flags: review?(bzbarsky) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: