Mounir Lamouri (:mounir) Assignee
	Description • 12 years ago

We need that so when creating the document's principal, we can pass to the SecurityManager the correct information.

	Mounir Lamouri (:mounir) Assignee
	Comment 3 • 12 years ago

If there is a browserParentURI + an app id, this will be considered as a non-installed content in a mozbrowser.

	Justin Lebar (not reading bugmail)
	Comment 5 • 12 years ago

Comment on attachment 639037 [details] [diff] [review]
Part 1 - Get browserOrigin from docshell

>diff --git a/docshell/base/nsIDocShell.idl b/docshell/base/nsIDocShell.idl
>index 8c74e1e..277c468b 100644
>--- a/docshell/base/nsIDocShell.idl
>+++ b/docshell/base/nsIDocShell.idl
>@@ -34,17 +34,17 @@ interface nsISHEntry;
> interface nsILayoutHistoryState;
> interface nsISecureBrowserUI;
> interface nsIDOMStorage;
> interface nsIPrincipal;
> interface nsIWebBrowserPrint;
> interface nsIVariant;
> interface nsIPrivacyTransitionObserver;
> 
>-[scriptable, uuid(6f60ac96-fa2c-41a5-92b4-29aaadbd7a7b)]
>+[scriptable, uuid(c98f0f21-fe96-4f06-9978-0a9422a789fa)]
> interface nsIDocShell : nsISupports
> {
>   /**
>    * Loads a given URI.  This will give priority to loading the requested URI
>    * in the object implementing	this interface.  If it can't be loaded here
>    * however, the URL dispatcher will go through its normal process of content
>    * loading.
>    *
>@@ -588,9 +588,22 @@ interface nsIDocShell : nsISupports
>    */
>   attribute bool isBrowserFrame;
> 
>   /*
>    * Is this docshell contained in an <iframe mozbrowser>, either directly or
>    * indirectly?
>    */
>   readonly attribute bool containedInBrowserFrame;
>+
>+  /**
>+   * Returns the URI of the parent of the <iframe mozbrowser>, if any.
>+   * Returns null if the docshell isn't part of an
>+   * <iframe mozbrowser> or if the browser frame has no parent.
>+   */
>+  readonly attribute nsIURI browserParentURI;

Sorry to bikeshed, but I don't like this name; it doesn't match nomenclature we use anywhere else.  How about browserOwnerURI, or perhaps browserEmbedderURI?

Also, please update the comment to include the following information:

 * docshells inside <iframe mozapp> do not have a browserEmbedderURI.
 * frames inside an <iframe mozbrowser> have the same browserOwnerURI as their parent.

But at a more basic level: How are you going to make this work cross-process?  The URI of the embedder can change (e.g. via pushState).  Maybe you should just return the origin here instead of the whole URI.

>+  const unsigned long NO_APP_ID = 0;

Can this be -1?  I know it's unsigned and all that, but if you use -1, I don't
have to check that the app service will never use app ID 0.

>+  [noscript] void setAppId(in unsigned long appId);

I see why you want the appId attribute to be separate from setAppId: if you do docshell.appId = docshell.appId, that has a real effect.  Can you please explain this in a comment?  Also, why is setAppId noscript?

>+  readonly attribute unsigned long appId;

Can you please explain that this is transitive, including through mozbrowser barriers?

In general, we have some of this functionality in nsGlobalWindow; this seems pretty repetitive.  It's fine to move it to docshell, but can we rip out the redundant code?

	Justin Lebar (not reading bugmail)
	Comment 6 • 12 years ago

I'd like to have another look at this whole queue once we figure out the questions above.  But the code in patches 2-4 is fine, for what it's trying to do.

	Justin Lebar (not reading bugmail)
	Comment 7 • 12 years ago

Oh, I see...the e10s work is in bug 770532.

	Mounir Lamouri (:mounir) Assignee
	Comment 8 • 12 years ago

(In reply to Justin Lebar [:jlebar] from comment #5)
> >+
> >+  /**
> >+   * Returns the URI of the parent of the <iframe mozbrowser>, if any.
> >+   * Returns null if the docshell isn't part of an
> >+   * <iframe mozbrowser> or if the browser frame has no parent.
> >+   */
> >+  readonly attribute nsIURI browserParentURI;
> 
> Sorry to bikeshed, but I don't like this name; it doesn't match nomenclature
> we use anywhere else.  How about browserOwnerURI, or perhaps
> browserEmbedderURI?

Ok.

> Also, please update the comment to include the following information:
> 
>  * docshells inside <iframe mozapp> do not have a browserEmbedderURI.
>  * frames inside an <iframe mozbrowser> have the same browserOwnerURI as
> their parent.

Ok.

> But at a more basic level: How are you going to make this work
> cross-process?

See bug 770532.

> The URI of the embedder can change (e.g. via pushState). 
> Maybe you should just return the origin here instead of the whole URI.

My first thought was to pass the origin but I'm actually passing the URI and I'm then using the origin of that URI to generate the dataId. It seems that nsScriptSecurityManager origin getter method handles way better the edge cases than the other places. Also, I thought that would prevent having us making a choice too early regarding how we generate the dataId. We might want to use the URI maybe, at some point.
This said, we can also have the nsScriptSecurityManager origin getter method public and pass the origin everywhere.

> >+  const unsigned long NO_APP_ID = 0;
> 
> Can this be -1?  I know it's unsigned and all that, but if you use -1, I
> don't
> have to check that the app service will never use app ID 0.

The app's service code can't return 0. I don't see how using -1 would make this better. I would prefer to keep 0. Even more given that we should be using constants so -1 or 0 shouldn't matter much.

> >+  [noscript] void setAppId(in unsigned long appId);
> 
> I see why you want the appId attribute to be separate from setAppId: if you
> do docshell.appId = docshell.appId, that has a real effect.  Can you please
> explain this in a comment?  Also, why is setAppId noscript?

This is to reduce the visibility of this method to only a part of our code and make it more complex to abuse it. I believe this is the reason why some other attributes/methods are using [noscript] in nsIDocShell.idl.

> >+  readonly attribute unsigned long appId;
> 
> Can you please explain that this is transitive, including through mozbrowser
> barriers?

Ok.

> In general, we have some of this functionality in nsGlobalWindow; this seems
> pretty repetitive.  It's fine to move it to docshell, but can we rip out the
> redundant code?

Sure. That should clearly happen. But, we can do that later as a follow-up.

	Justin Lebar (not reading bugmail)
	Comment 9 • 12 years ago

> My first thought was to pass the origin but I'm actually passing the URI and
> I'm then using the origin of that URI to generate the dataId. It seems that
> nsScriptSecurityManager origin getter method handles way better the edge cases
> than the other places. 

Passing around the full URI might make sense from the point of view of your
principal code, but it doesn't make sense to have a docshell method that "gets
the URI of the browser embedder" if that URI can be stale.

> Also, I thought that would prevent having us making a
> choice too early regarding how we generate the dataId. We might want to use
> the URI maybe, at some point.

In the presence of pushState, you can't use anything more than the origin to
identify a document.

What edge cases are you trying to avoid here?

>> >+  const unsigned long NO_APP_ID = 0;
>> 
>> Can this be -1?  I know it's unsigned and all that, but if you use -1, I
>> don't
>> have to check that the app service will never use app ID 0.
>
>The app's service code can't return 0. I don't see how using -1 would make this better. I would prefer to keep 0. Even more given that we should be using constants so -1 or 0 shouldn't matter much.

Okay.

>> >+  [noscript] void setAppId(in unsigned long appId);
>> 
>> I see why you want the appId attribute to be separate from setAppId: if you
>> do docshell.appId = docshell.appId, that has a real effect.  Can you please
>> explain this in a comment?  Also, why is setAppId noscript?
>
>This is to reduce the visibility of this method to only a part of our code and make it more complex to abuse it. I believe this is the reason why some other attributes/methods are using [noscript] in nsIDocShell.idl.

I hit something like this in nsIWindowUtils -- there was a noscript method I wanted to call from script, and then I had to figure out whether it was [noscript] because it was /unsafe/, or just because the author thought it was "too complicated" for script.

You're already calling the nsGlobalWindow setAppId function from script; it doesn't seem like a stretch to call this one from script either...  But whatever you prefer; it's not a big deal.

>> In general, we have some of this functionality in nsGlobalWindow; this seems
>> pretty repetitive.  It's fine to move it to docshell, but can we rip out the
>> redundant code?
>
>Sure. That should clearly happen. But, we can do that later as a follow-up.

Well, I'd prefer to see this change now, because it's not entirely clear to me that the docshell interface subsumes the window interface.  That is, I'd rather not land this code, try to remove the window code, realize we can't remove it, and then have to rework this interface /again/.  If you want to do it as a follow-up bug and land it together with this one, that's fine with me.  Or if you're confident that the window code can easily be stripped out, then that's fine too.


Separately, I wonder if we should call it "embedderURI" and provide it for both browsers /and/ apps.  Then we'd have three pieces of state: is-app, is-browser, and embedder-uri.  Is-app and is-browser are mutually-exclusive (and you could use an enum if you wanted).

The reason for this is that as written, we bake the assumption that we never care about the app's embedderURI into all of our code.  But that's not a future-proof assumption: Just as browser X inside browser Y is different from browser X inside browser Z, we might reasonably decide that app X inside app Y is different from app X inside app Z.

It's no harder to do it this way, I think; in fact, I think it would be cleaner, since then you wouldn't have the implicit assumption that !!browserEmbedderURI iff we-are-a-browser.  Explicit is better than implicit and all that.

	Mounir Lamouri (:mounir) Assignee
	Comment 13 • 12 years ago

I have tests for that. I don't know if I will attach them here or somewhere else though.

	Justin Lebar (not reading bugmail)
	Comment 14 • 12 years ago

># HG changeset patch
># Parent 4fd0671476277dfe248e35e39567434de8171f0b
># User Mounir Lamouri <mounir.lamouri@gmail.com>
># Date 1342574451 25200
>
>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>--- a/content/base/src/nsFrameLoader.cpp
>+++ b/content/base/src/nsFrameLoader.cpp
>@@ -1455,16 +1456,34 @@ nsFrameLoader::MaybeCreateDocShell()
>     doc->GetContainer();
>   nsCOMPtr<nsIWebNavigation> parentAsWebNav = do_QueryInterface(container);
>   NS_ENSURE_STATE(parentAsWebNav);
> 
>   // Create the docshell...
>   mDocShell = do_CreateInstance("@mozilla.org/docshell;1");
>   NS_ENSURE_TRUE(mDocShell, NS_ERROR_FAILURE);
> 
>+  if (OwnerIsBrowserFrame() &&
>+      mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::mozapp)) {
>+    nsCOMPtr<nsIAppsService> appsService =
>+      do_GetService(APPS_SERVICE_CONTRACTID);
>+    if (!appsService) {
>+      NS_ERROR("Apps Service is not available!");
>+      return NS_ERROR_FAILURE;
>+    }
>+
>+    nsAutoString manifest;
>+    mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::mozapp, manifest);
>+
>+    PRUint32 appId;
>+    appsService->GetAppLocalIdByManifestURL(manifest, &appId);
>+
>+    mDocShell->SetAppId(appId);
>+  }

Only in-process docshells go through nsFrameLoader::MaybeCreateDocShell.  What about OOP docshells?

>+NS_IMETHODIMP
>+nsDocShell::GetAppId(PRUint32* aAppId)
>+{
>+    if (mAppId != nsIScriptSecurityManager::NO_APP_ID) {
>+#ifdef DEBUG
>+        PRUint16 type;
>+        GetExactFrameType(&type);
>+        MOZ_ASSERT(type == nsIDocShell::FRAME_TYPE_APP);
>+#endif // DEBUG
>+        *aAppId = mAppId;
>+        return NS_OK;
>+    }
>+
>+#ifdef DEBUG
>+        PRUint16 type;
>+        GetExactFrameType(&type);
>+        MOZ_ASSERT(type != nsIDocShell::FRAME_TYPE_APP);
>+#endif // DEBUG

Indentation inside the ifdef should match indentation outside the ifdef.  Or
alternatively, indent inside the first ifdef.

>diff --git a/docshell/base/nsIDocShell.idl b/docshell/base/nsIDocShell.idl
>--- a/docshell/base/nsIDocShell.idl
>+++ b/docshell/base/nsIDocShell.idl
>@@ -34,17 +34,17 @@ interface nsISHEntry;
> interface nsILayoutHistoryState;
> interface nsISecureBrowserUI;
> interface nsIDOMStorage;
> interface nsIPrincipal;
> interface nsIWebBrowserPrint;
> interface nsIVariant;
> interface nsIPrivacyTransitionObserver;
> 
>-[scriptable, builtinclass, uuid(57889367-590b-4ea2-a345-5211253babf5)]
>+[scriptable, builtinclass, uuid(c98f0f21-fe96-4f06-9978-0a9422a789fa)]
> interface nsIDocShell : nsISupports
> {
>   /**
>    * Loads a given URI.  This will give priority to loading the requested URI
>    * in the object implementing	this interface.  If it can't be loaded here
>    * however, the URL dispatcher will go through its normal process of content
>    * loading.
>    *
>@@ -608,9 +608,28 @@ interface nsIDocShell : nsISupports
>    * parent with some useful information is found.
>    */
>   readonly attribute unsigned short frameType;
> 
>   /**
>    * Returns the type of the frame without checking the parent chain.
>    */
>   readonly attribute unsigned short exactFrameType;
>+
>+  /**
>+   * Set the app id this docshell is associated with. It has to be a valid app
>+   * id. If the docshell isn't associated with any app, the value should be
>+   * nsIScriptSecurityManager::NO_APP_ID.

Nit: s/It/The id/.  Also, if this is all one paragraph, "However" shouldn't be
on a new line.

>+   * However, this is the default value if nothing is set.
>+   *
>+   * This method is [noscript] to reduce the scope. It should be used at very
>+   * specific moments.
>+   *
>+   * Calling setAppId() will mark the frame as a FRAME_TYPE_APP.
>+   */
>+  [noscript] void setAppId(in unsigned long appId);
>+
>+  /**
>+   * Returns the app id of the app the docshell is in.
>+   * Returns nsIScriptSecurityManager::NO_APP_ID If the docshell is not in an app.
>+   */
>+  readonly attribute unsigned long appId;

Nit: s/If/if, and don't add a linebreak before the second "Returns" (or
alternatively add a fully blank line).

	Justin Lebar (not reading bugmail)
	Comment 15 • 12 years ago

> Only in-process docshells go through nsFrameLoader::MaybeCreateDocShell.  What about OOP docshells?

Again I missed the transitive blocking of bug 770532; I've made it explicit now.  :)

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 16 • 12 years ago

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

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

::: docshell/base/nsIDocShell.idl
@@ +624,5 @@
> +   * specific moments.
> +   *
> +   * Calling setAppId() will mark the frame as a FRAME_TYPE_APP.
> +   */
> +  [noscript] void setAppId(in unsigned long appId);

I'm not a fan of having this function called 'setAppId' since it looks like a getter/setter pair together with the appId getter. But since Justin r+'ed it I'm ok with it.

	Mounir Lamouri (:mounir) Assignee
	Comment 17 • 12 years ago

Marking in-testsuite+ because I have tests for that. They will be in bug 758258.

	Justin Lebar (not reading bugmail)
	Comment 18 • 12 years ago

> I'm not a fan of having this function called 'setAppId' since it looks like a getter/setter pair 
> together with the appId getter.

I think that's exactly what mounir wanted, except with a noscript setter?  Personally, I don't see much use in this being noscript, but that is not a battle I choose to wage at this time.  :)

	Steve Fink [:sfink] [:s:]
	Comment 19 • 12 years ago

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/c2ffcbf39231
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/8311845bd51b

(build appears to depend on bug 758258 that I backed out)

	Ed Morley [:emorley]
	Comment 20 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/159878ae4115