774957 - Update nsDocShell jazz so distinction between browser and app frame is clearer

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 643213 [details] [diff] [review]
Patch

Review of attachment 643213 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsIDocShell.idl
@@ +600,5 @@
> +  const unsigned short FRAME_TYPE_APP      = 0x2; // 0010
> +  // FRAME_TYPE_EMBEDDER is a mask for all types that behave as embedder. That
> +  // means that they act like a content boundary for the purpose of window.top
> +  // and window.parent.
> +  const unsigned short FRAME_TYPE_EMBEDDER = 0x3; // 0011

This seems overly complicated. Why isn't there just 3 types:

<iframe>                   -> regular
<iframe mozbrowser>        -> browser
<iframe mozbrowser mozapp> -> app

Comment 2

[Mounir Lamouri (:mounir)]

(In reply to Jonas Sicking (:sicking) from comment #1)
> ::: docshell/base/nsIDocShell.idl
> @@ +600,5 @@
> > +  const unsigned short FRAME_TYPE_APP      = 0x2; // 0010
> > +  // FRAME_TYPE_EMBEDDER is a mask for all types that behave as embedder. That
> > +  // means that they act like a content boundary for the purpose of window.top
> > +  // and window.parent.
> > +  const unsigned short FRAME_TYPE_EMBEDDER = 0x3; // 0011
> 
> This seems overly complicated. Why isn't there just 3 types:
> 
> <iframe>                   -> regular
> <iframe mozbrowser>        -> browser
> <iframe mozbrowser mozapp> -> app

Because a lot of consumers actually want to know if it's "browser | app".

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

They can just check if |type != regular|

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to Jonas Sicking (:sicking) from comment #3)
> They can just check if |type != regular|

It's

Comment 5

[Mounir Lamouri (:mounir)]

... less future-proof.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If we update the set of types that we can have then more likely than not we need to audit all code that uses the type anyway.

If we want to be future proof we need to break out the semantic meanings, such as "is process boundary", "is app boundary", "is data jar boundary" etc.

I prefer to keep it simple and audit the clients if we need to.

Comment 7

[Justin Lebar (not reading bugmail)]

>> This seems overly complicated. Why isn't there just 3 types:
>
> Because a lot of consumers actually want to know if it's "browser | app".

I agree with Mounir here that cleanly supporting (browser | app) is important; at the moment, the vast majority of callers care only about that.  And I don't like using "type != regular" for (browser | app), because explicit is better than implicit, and our docshell code is confusing enough as-is.

But I agree with Jonas that this API is non-ideal.  As evidence, consider: 
 
> +  PRUint16 frameType;
> +  mDocShell->GetExactFrameType(&frameType);
> +  if (frameType == nsIDocShell::FRAME_TYPE_EMBEDDER) {

frameType is never equal to EMBEDDER -- you need to do |if (frameType & FRAME_TYPE_EMBEDDER)|, right?  In fact, this patch never uses EMBEDDER correctly, as far as I can tell.

I like the idea of breaking this out into separate methods.  For example:

  IsBrowserFrame
  IsAppFrame
  IsBounadryFrame

  // These search up the parent chain.
  IsInheritedBrowserFrame
  IsInheritedAppFrame
  IsInheritedBoundaryFrame

But I think focusing on the "frame" part may be wrong here.  So perhaps instead:

  Is{Inherited,}Browser
  Is{Inherited,}App
  Is{Inherited,}ContentBoundary

?

Comment 8

[Justin Lebar (not reading bugmail)]

Comment on attachment 643213 [details] [diff] [review]
Patch

r- for bugs, even if we agreed to keep this interface.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Having spent a few hours on the naming, we came to the following consensus:

 .isBrowser
 .isApp
 .isContentBoundary
 .appId
 .inBrowserElement

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

...and there was much celebration and festivities. Meed was drunk and roast beast was scarfed.

Comment 11

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

What about that?

Comment 12

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

I'm glad I don't have to pay a beer everything I forget to qref ;)

Comment 13

[Justin Lebar (not reading bugmail)]

Comment on attachment 643627 [details] [diff] [review]
Patch v2

>+nsDocShell::SetIsBrowser()
>+{
>+    if (mIsBrowserFrame) {
>+        NS_ERROR("You should not call SetIsBrowser() more than once.");
>+        return NS_OK;
>+    }
>+
>+    mIsBrowserFrame = true;
>+
>     nsCOMPtr<nsIObserverService> os = services::GetObserverService();
>     if (os) {
>+        os->NotifyObservers(GetAsSupports(this),
>+                            "docshell-marked-as-browser-frame", NULL);
>+    }

Nobody is listening to this observer; we can get rid of it.  But if you want to
do so separately (or want to make me do it), that's OK with me.

>+nsDocShell::FrameType
>+nsDocShell::GetInheritedFrameType() const
>+{
>+    FrameType type = GetFrameType();
>+
>+    if (type != eFrameTypeRegular) {
>+        return type;
>     }
> 
>     nsCOMPtr<nsIDocShellTreeItem> parentAsItem;
>-    GetSameTypeParent(getter_AddRefs(parentAsItem));
>+    const_cast<nsDocShell*>(this)->GetSameTypeParent(getter_AddRefs(parentAsItem));

I can't say I'm wild about this.  nsDocShell is not const-correct, and nobody
holds |const nsDocShell*|'s.  But if you do want GetInheritedFrameType to be
const, then I'd prefer you made all the methods that GetSameTypeParent calls
also be const.

>diff --git a/docshell/base/nsDocShell.h b/docshell/base/nsDocShell.h
>--- a/docshell/base/nsDocShell.h
>+++ b/docshell/base/nsDocShell.h
>@@ -659,16 +659,30 @@ protected:
>+    enum FrameType {
>+        eFrameTypeRegular  = 0x0, // 0000
>+        eFrameTypeBrowser  = 0x1, // 0001
>+        eFrameTypeApp      = 0x2  // 0010
>+    };
>+
>+    // FrameTypeBoundaryMask is a mask for all types that behave as content
>+    // boundary. That means that they act like a content boundary for the
>+    // purpose of window.top and window.parent.
>+    static const PRUint16 FrameTypeBoundaryMask = 0x3; // 0011

I think it would be simpler not to have this and just do a boolean |or| in the
docshell code.

>@@ -584,26 +584,47 @@ interface nsIDocShell : nsISupports
>+  /**
>+   * Mark the docshell as a browser frame.
>+   * That should be used for <iframe mozbrowser> but not for <iframe mozapp>.
>    */
>-  attribute bool isBrowserFrame;
>+  void setIsBrowser();

s/That/This/.  Also, this will crash if you call it more than once, so you
should probably say that in the interface.  :)

>-  /*
>-   * Is this docshell contained in an <iframe mozbrowser>, either directly or
>-   * indirectly?
>+  /**
>+   * Returns if the docshell is marked as a browser frame.
>    */
>-  readonly attribute bool containedInBrowserFrame;
>+  readonly attribute boolean isBrowser;

"Returns if" used this way is not terribly idiomatic.  It kind of implies "this
function returns if X, and otherwise it doesn't return".  No matter how you try
to phrase this, it's going to be awkward, so what you have is OK, except when
we come to isInApp...

You could say "Returns true iff" (iff means if and only if).

>+  /**
>+   * Returns if the docshell is marked as an app frame.
>+   */
>+  readonly attribute boolean isApp;

>+  /**
>+   * Returns if the docshell is known to be a content boundary frame.
>+   */
>+  readonly attribute boolean isContentBoundary;

I don't know what it means to be "known to be a content boundary frame".

>+  /**
>+   * Returns if the docshell is inside a browser element.
>+   */
>+  readonly attribute boolean isInBrowserElement;

Why is it isBrowser, but isInBrowserElement?

>+  /**
>+   * Returns if the docshell is inside an application.
>+   * Returns false if the docshell is inside a browser element that is inside an
>+   * application.
>+   */
>+  readonly attribute boolean isInApp;

Returns if...
Returns /false/ if...

Now this is a lack of parallelism I can't abide.  :)

>+  /**
>+   * Returns if the docshell has a docshell that behaves as a content boundary
>+   * in his parent hierarchy.
>+   */
>+  readonly attribute boolean isInContentBoundary;

I'm sorry to nit further on these names, but it really doesn't make much sense
to say that the docshell is "in" a content boundary.

Can we replace all these "in"s with "below"s?  "isBelowApp" is not great, but
considering that isBelowContentBoundary will be called /far/ more often, I
think that's an OK trade-off.

>diff --git a/toolkit/components/social/FrameWorker.jsm b/toolkit/components/social/FrameWorker.jsm
>--- a/toolkit/components/social/FrameWorker.jsm
>+++ b/toolkit/components/social/FrameWorker.jsm
>@@ -230,17 +230,17 @@ function makeHiddenFrame() {
>   let docShell = iframe.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDocShell);
>   docShell.allowAuth = false;
>   docShell.allowPlugins = false;
>   docShell.allowImages = false;
>   docShell.allowWindowControl = false;
>   // TODO: disable media (bug 759964)
>   
>   // Mark this docShell as a "browserFrame", to break script access to e.g. window.top
>-  docShell.isBrowserFrame = true;
>+  docShell.setIsBrowser();

Sigh; this only works if nobody ever reads the attribute off the iframe.  How
much you want to bet that doesn't work, or that we'll break it at some point?

r=me; if necessary, I'll go in to nsDocShell and do an OCD pass on the comments
later.

Comment 14

[Justin Lebar (not reading bugmail)]

> if necessary, I'll go in to nsDocShell and do an OCD pass on the comments later.

I mean, in a separate bug, after you've checked this in.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 643627 [details] [diff] [review]
Patch v2

Review of attachment 643627 [details] [diff] [review]:
-----------------------------------------------------------------

I would really prefer to call the security property "inBrowserElement" since that will be used in a lot of places. But we can do that as a followup if we all can get to an agreement.

Comment 16

[Justin Lebar (not reading bugmail)]

> I would really prefer to call the security property "inBrowserElement"

As opposed to "isInBrowserElement", or something else?

> since that will be used in a lot of places.

Because it's shorter?

Comment 17

[Mounir Lamouri (:mounir)]

I fixed all of the comments. For the naming, I chose to do the following:
IsBrowserElement
IsApp
IsContentBoundary
IsInBrowserElement
IsInApp
IsBelowContentBoundary

I'm going to open a follow-up for that in case of.

Comment 18

[Steve Fink [:sfink] [:s:]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/060a9d9fc1c6
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/c06d60a53870

(appears to depend on bug 758258 that I had to backout)

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d1bb0c6f34b5