The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla17

Status

()

Core
Document Navigation
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Unassigned)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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).
(Reporter)

Updated

5 years ago
Blocks: 693515
(Reporter)

Comment 1

5 years ago
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!
(Reporter)

Updated

5 years ago
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
(Reporter)

Updated

5 years ago
Depends on: 764718
(Reporter)

Updated

5 years ago
Blocks: 769182
(Reporter)

Comment 2

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=f05856f40d6a
(Reporter)

Comment 3

5 years ago
Created attachment 639924 [details] [diff] [review]
Part 1: Make nsDocShell::GetSameTypeParent and friends respect <iframe mozbrowser> boundaries.

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)
(Reporter)

Comment 4

5 years ago
Created attachment 639925 [details] [diff] [review]
Part 2: Make in-process <iframe mozbrowser>'s have their own shistory objects.
Attachment #639925 - Flags: review?(bzbarsky)
(Reporter)

Comment 5

5 years ago
Created attachment 639926 [details] [diff] [review]
Part 3: Test that window.open(..., 'top') works properly for in-process <iframe mozbrowser>.

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)
(Reporter)

Comment 6

5 years ago
Created attachment 639927 [details] [diff] [review]
Part 4: Enable BackForward test for in-process <iframe mozbrowser>, now that it works.
Attachment #639927 - Flags: review?(bzbarsky)
(Reporter)

Updated

5 years ago
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+
(Reporter)

Comment 11

5 years ago
> 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.
(Reporter)

Comment 12

5 years ago
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?
(Reporter)

Comment 14

5 years ago
> 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?
(Reporter)

Comment 16

5 years ago
> 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.  ;)
(Reporter)

Comment 18

5 years ago
>> 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...
(Reporter)

Comment 19

5 years ago
Created attachment 641904 [details] [diff] [review]
Part 1, v2: Make nsDocShell::GetSameTypeParent and friends respect <iframe mozbrowser> boundaries.
Attachment #641904 - Flags: review?(bzbarsky)
(Reporter)

Comment 20

5 years ago
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)
(Reporter)

Updated

5 years ago
Attachment #639924 - Attachment is obsolete: true
(Reporter)

Comment 21

5 years ago
Created attachment 641906 [details] [diff] [review]
Part 5: Add test for window.content inside <iframe mozbrowser>.
Attachment #641906 - Flags: review?(bzbarsky)
> If it's OK with you, I'd like to handle this in a follow-up. 

Worksforme.
(Reporter)

Updated

5 years ago
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+
https://hg.mozilla.org/mozilla-central/rev/3bfdf04d02f4
https://hg.mozilla.org/mozilla-central/rev/307af50437ef
https://hg.mozilla.org/mozilla-central/rev/eb1e4ede61f1
https://hg.mozilla.org/mozilla-central/rev/ed117c0cde36
https://hg.mozilla.org/mozilla-central/rev/edc0540d1c3e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.