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

RESOLVED FIXED in mozilla17

Status

()

Core
Document Navigation
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Blocks: 1 bug)

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

18.36 KB, patch
Justin Lebar (not reading bugmail)
: review+
sicking
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Created attachment 643213 [details] [diff] [review]
Patch
Attachment #643213 - Flags: superreview?(jonas)
Attachment #643213 - Flags: review?(justin.lebar+bug)
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
(Assignee)

Comment 2

5 years ago
(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".
They can just check if |type != regular|
(Assignee)

Comment 4

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

It's
(Assignee)

Comment 5

5 years ago
... less future-proof.
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.
>> 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 on attachment 643213 [details] [diff] [review]
Patch

r- for bugs, even if we agreed to keep this interface.
Attachment #643213 - Flags: review?(justin.lebar+bug) → review-
Having spent a few hours on the naming, we came to the following consensus:

 .isBrowser
 .isApp
 .isContentBoundary
 .appId
 .inBrowserElement
...and there was much celebration and festivities. Meed was drunk and roast beast was scarfed.
(Assignee)

Comment 11

5 years ago
Created attachment 643625 [details] [diff] [review]
Patch v2

What about that?
Attachment #643213 - Attachment is obsolete: true
Attachment #643213 - Flags: superreview?(jonas)
Attachment #643625 - Flags: superreview?(jonas)
Attachment #643625 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 12

5 years ago
Created attachment 643627 [details] [diff] [review]
Patch v2

I'm glad I don't have to pay a beer everything I forget to qref ;)
Attachment #643625 - Attachment is obsolete: true
Attachment #643625 - Flags: superreview?(jonas)
Attachment #643625 - Flags: review?(justin.lebar+bug)
Attachment #643627 - Flags: superreview?(jonas)
Attachment #643627 - Flags: review?(justin.lebar+bug)
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.
Attachment #643627 - Flags: review?(justin.lebar+bug) → review+
> 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 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.
Attachment #643627 - Flags: superreview?(jonas) → superreview+
> 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?
(Assignee)

Comment 17

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

Updated

5 years ago
Depends on: 775408
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)
https://hg.mozilla.org/mozilla-central/rev/d1bb0c6f34b5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.