Last Comment Bug 771273 - Make nsDocShell::GetSameTypeParent and friends respect <iframe mozbrowser> boundaries
: Make nsDocShell::GetSameTypeParent and friends respect <iframe mozbrowser> bo...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 764718 772076
Blocks: browser-api 773675 769182
  Show dependency treegraph
 
Reported: 2012-07-05 12:32 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-07-24 03:00 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Make nsDocShell::GetSameTypeParent and friends respect <iframe mozbrowser> boundaries. (7.64 KB, patch)
2012-07-06 21:08 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review-
Details | Diff | Review
Part 2: Make in-process <iframe mozbrowser>'s have their own shistory objects. (1.42 KB, patch)
2012-07-06 21:08 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review+
Details | Diff | Review
Part 3: Test that window.open(..., 'top') works properly for in-process <iframe mozbrowser>. (5.26 KB, patch)
2012-07-06 21:09 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review+
Details | Diff | Review
Part 4: Enable BackForward test for in-process <iframe mozbrowser>, now that it works. (3.37 KB, patch)
2012-07-06 21:09 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review+
Details | Diff | Review
Part 1, v2: Make nsDocShell::GetSameTypeParent and friends respect <iframe mozbrowser> boundaries. (7.52 KB, patch)
2012-07-13 08:14 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review+
Details | Diff | Review
Part 5: Add test for window.content inside <iframe mozbrowser>. (1.85 KB, patch)
2012-07-13 08:16 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review+
Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2012-07-05 12:32:39 PDT
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).
Comment 1 Justin Lebar (not reading bugmail) 2012-07-05 13:49:09 PDT
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!
Comment 2 Justin Lebar (not reading bugmail) 2012-07-06 15:42:27 PDT
https://tbpl.mozilla.org/?tree=Try&rev=f05856f40d6a
Comment 3 Justin Lebar (not reading bugmail) 2012-07-06 21:08:09 PDT
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.
Comment 4 Justin Lebar (not reading bugmail) 2012-07-06 21:08:25 PDT
Created attachment 639925 [details] [diff] [review]
Part 2: Make in-process <iframe mozbrowser>'s have their own shistory objects.
Comment 5 Justin Lebar (not reading bugmail) 2012-07-06 21:09:26 PDT
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...
Comment 6 Justin Lebar (not reading bugmail) 2012-07-06 21:09:38 PDT
Created attachment 639927 [details] [diff] [review]
Part 4: Enable BackForward test for in-process <iframe mozbrowser>, now that it works.
Comment 7 Boris Zbarsky [:bz] 2012-07-10 14:52:12 PDT
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.
Comment 8 Boris Zbarsky [:bz] 2012-07-10 14:52:56 PDT
Comment on attachment 639925 [details] [diff] [review]
Part 2: Make in-process <iframe mozbrowser>'s have their own shistory objects.

r=me
Comment 9 Boris Zbarsky [:bz] 2012-07-10 14:53:28 PDT
Comment on attachment 639926 [details] [diff] [review]
Part 3: Test that window.open(..., 'top') works properly for in-process <iframe mozbrowser>.

r=me
Comment 10 Boris Zbarsky [:bz] 2012-07-10 14:54:01 PDT
Comment on attachment 639927 [details] [diff] [review]
Part 4: Enable BackForward test for in-process <iframe mozbrowser>, now that it works.

r=me
Comment 11 Justin Lebar (not reading bugmail) 2012-07-11 13:56:22 PDT
> 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.
Comment 12 Justin Lebar (not reading bugmail) 2012-07-11 15:17:31 PDT
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.
Comment 13 Boris Zbarsky [:bz] 2012-07-11 19:32:32 PDT
> 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?
Comment 14 Justin Lebar (not reading bugmail) 2012-07-12 08:30:50 PDT
> 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.
Comment 15 Boris Zbarsky [:bz] 2012-07-12 19:05:33 PDT
> 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?
Comment 16 Justin Lebar (not reading bugmail) 2012-07-12 23:17:03 PDT
> 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?
Comment 17 Boris Zbarsky [:bz] 2012-07-13 02:03:00 PDT
Sounds plausible.  ;)
Comment 18 Justin Lebar (not reading bugmail) 2012-07-13 08:11:57 PDT
>> 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 19 Justin Lebar (not reading bugmail) 2012-07-13 08:14:45 PDT
Created attachment 641904 [details] [diff] [review]
Part 1, v2: Make nsDocShell::GetSameTypeParent and friends respect <iframe mozbrowser> boundaries.
Comment 20 Justin Lebar (not reading bugmail) 2012-07-13 08:15:51 PDT
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)
Comment 21 Justin Lebar (not reading bugmail) 2012-07-13 08:16:55 PDT
Created attachment 641906 [details] [diff] [review]
Part 5: Add test for window.content inside <iframe mozbrowser>.
Comment 22 Boris Zbarsky [:bz] 2012-07-13 09:07:15 PDT
> If it's OK with you, I'd like to handle this in a follow-up. 

Worksforme.
Comment 23 Boris Zbarsky [:bz] 2012-07-13 15:06:56 PDT
Comment on attachment 641904 [details] [diff] [review]
Part 1, v2: Make nsDocShell::GetSameTypeParent and friends respect <iframe mozbrowser> boundaries.

r=me
Comment 24 Boris Zbarsky [:bz] 2012-07-13 15:07:31 PDT
Comment on attachment 641906 [details] [diff] [review]
Part 5: Add test for window.content inside <iframe mozbrowser>.

r=me

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