Closed Bug 1227861 Opened 9 years ago Closed 8 years ago

Add OriginAttributes getter/setter into nsIDocShell

Categories

(Core :: Security: CAPS, defect)

Product:
Core
Component:
Security: CAPS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox45 --- affected
firefox47 --- fixed

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 12 obsolete files)

add origin attributes into nsIDocShell. v8
29.25 KB, patch
Details | Diff | Splinter Review 
See Jonas' comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1209162#c46

also we need to modify nsDocShell::GetOriginAttributes for this.
Assignee: nobody → allstars.chh
Hi, Jonas
Do you want to add this method into nsDocShell or nsIDocShell.idl ?

Because I added originAttributes into nsILoadContext.idl in Bug 1165466
However nsDocShell implements nsIDocShell and nsILoadContext, so if we want to add this into nsIDocShell, it might make things more complicated, for example, originAttributes is a read-only attributes in nsILoadContext but has a setter in nsIDocShell.
Flags: needinfo?(jonas)
If you name the attribute 'docShellOriginAttributes' in nsIDocShell, then it should be no problem
Flags: needinfo?(jonas)
Summary: Add nsDocShell::SetOriginAttributes → Add docShellOriginAttributes in nsIDocShell
Comment on attachment 8694145 [details] [diff] [review]
WIP patch.

Hi Jonas
Could you take a look on this?
I didn't remove those SetIsApp and SetIsBrowser... functions from nsIDocShell, as I found out some Desktop code will use set/getIsApp.

Feel free to comment if you have other thoughts.

Thanks
Attachment #8694145 - Flags: feedback?(jonas)
I might remove SetIsPackage/SetIsApp/SetIsBrowserInsideApp from nsIDocShell, but not sure should I do the same for those getters like isBrowserElement, isApp, isBrowserOrApp?

For checking isBrowserElement is easy, they could just check the mInBrowsr flag from OriginAttributes, however for isApp and isBrowserOrApp, callers need to check if it's not NO_APP_ID nor UNKNOWN_APP_ID as well.

not sure if I should do it here, comments are welcome :D
Comment on attachment 8694145 [details] [diff] [review]
WIP patch.

spotted some problems
Attachment #8694145 - Flags: feedback?(jonas)
Attached patch Patch. (obsolete) — — Splinter Review 

        Attachment #8694145 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch. (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8695734 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8695741 [details] [diff] [review]
Patch.

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

In the end I still keep SetIsApp and SetIsBrowserInsideApp because I realized these two functions will set the mFrameType properly.

For example, an iframe inside <iframe mozbrowser mozapp> should have a valid 'appId', but its mFrameType should still remain 'regular'
However I did removed SetIsPackage and SetIsUserContextId from nsIDocShell.

        Attachment #8695741 -
        Flags: review?(jonas)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        rename to originAttributes (obsolete)
        — 
          — Splinter Review
      

    


  
rename it back to originAttributes if we think it's easier.

    
    

    
  
Comment on attachment 8695741 [details] [diff] [review]
Patch.

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

This is definitely a step in the right direction. However I still think that there's a few things we need to do:

Why do we still have setIsApp and setIsBrowserInsideApp on docshell? Doesn't that mean that we now store two appids in docshell? That seems bad since they can get out of sync.

Similarly, why do we need the docShell->SetIsBrowserInsideApp and docShell->SetIsApp call in TabChild::NotifyTabContextUpdated? Does this mean that TabChild also tracks two types of appids?

Generally though it seems like this patch is definitely in the right direction. So if you prefer we can land this as-is and remove the functions from docshell as a followup (though you'll need to merge with the changes in bug 1195881 which just landed).

Let me know.

    
    

    
  
> In the end I still keep SetIsApp and SetIsBrowserInsideApp because I
> realized these two functions will set the mFrameType properly.
> 
> For example, an iframe inside <iframe mozbrowser mozapp> should have a valid
> 'appId', but its mFrameType should still remain 'regular'
> However I did removed SetIsPackage and SetIsUserContextId from nsIDocShell.

Ooh. I see. We should figure out a better solution for that.

Could you add a SetFrameType function on nsIDocShell instead?

    
    

    
  
Comment on attachment 8695741 [details] [diff] [review]
Patch.

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

So yeah, let's add something like SetFrameType instead.

        Attachment #8695741 -
        Flags: review?(jonas) → review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch. v2 (obsolete)
        — 
          — Splinter Review
      

    


  
added frameType into nsIDocShell

        Attachment #8695741 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8708886 [details] [diff] [review]
Patch. v2

Added frameType into nsIDocShell.
And since I use [infallible] for frameType, XPIDL doesn't allow me to do 'typedef unsigned long FrameType' for it.

        Attachment #8708886 -
        Flags: review?(jonas)

    
    

    
  
Comment on attachment 8708886 [details] [diff] [review]
Patch. v2

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

This is looking really great! So much easier to follow and more generic than the existing code.

r=me with the below fixed.

::: browser/components/sessionstore/SessionHistory.jsm
@@ +255,5 @@
>      let webNavigation = docShell.QueryInterface(Ci.nsIWebNavigation);
>      let history = webNavigation.sessionHistory;
>  
>      if ("userContextId" in tabData) {
> +      docShell.docShellOriginAttributes = {userContextId: tabData.userContextId};

This will overwrite all the other attributes, that does not seem good.

How about something like:

attrs = docShell.docShellOriginAttributes;
attrs.userContextId = tabData.userContextId;
docShell.docShellOriginAttributes = attrs;

::: docshell/base/nsIDocShell.idl
@@ +1077,3 @@
>     */
> +  [implicit_jscontext]
> +  attribute jsval docShellOriginAttributes;

Can we make this into a getter function and a setter function instead.

When it's an attribute you'd generally expect that

docshell.docShellOriginAttributes == docshell.docShellOriginAttributes;

would return true, but that's not the case since the getter creates a new object each time. You'd also expect

docshell.docShellOriginAttributes.userContextId = x;

to work.

If we make this a

getDocShellOriginAttributes();
setDocShellOriginAttributes();

pair of functions instead, it's more clear how they work.

::: dom/base/nsFrameLoader.cpp
@@ +1897,5 @@
> +  //Grab the userContextId from owner if XUL
> +  nsAutoString userContextIdStr;
> +  if ((namespaceID == kNameSpaceID_XUL) &&
> +      mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::usercontextid)) {
> +    mOwnerContent->GetAttr(kNameSpaceID_None,

I know you're just moving this code. But could you fix this to be:

if ((namespaceID == kNameSpaceID_XUL) &&
    mOwnerContent->GetAttr(kNameSpaceID_None,
                           nsGkAtoms::usercontextid,
                           userContextIdStr) &&
    !userContextIdStr.IsEmpty()) {

GetAttr returns false if the attribute was not set.

@@ +1905,5 @@
>  
> +  if (!userContextIdStr.IsEmpty()) {
> +    nsresult rv;
> +    attrs.mUserContextId = userContextIdStr.ToInteger(&rv);
> +    NS_ENSURE_SUCCESS(rv, rv);

Put this instead of the if-statement above.

::: dom/ipc/TabChild.cpp
@@ +835,5 @@
> +                           nsIDocShell::FRAME_TYPE_BROWSER :
> +                           HasOwnApp() ?
> +                            nsIDocShell::FRAME_TYPE_APP :
> +                            nsIDocShell::FRAME_TYPE_REGULAR);
> +  nsDocShell::Cast(docShell)->SetOriginAttributes(OriginAttributesRef());

I assume that you've checked that this does the right thing for the various mozbrowser types? The current code is complex enough that I'm not sure by just reading the code.

        Attachment #8708886 -
        Flags: review?(jonas) → review+

    
    

    
  
I found a conflict when I update to latest m-c, from Bug 1239420.
However I'd like to confirm with Jonas and Baku first,

Jonas has suggested we use inheritance for origin attributes back in Bug 1209162
And in this bug, when the DocShell is created, it will inherit the origin attributes of the document from the parent DocShell.

However in Bug 1239420, when a user context id is set, it will also set the user context id to the child DocShell, that makes me confused about the inheritance of user context id, it looks to me we should leave userContextId out of those Inheritance functions and set it dynamically?

Thanks
Flags: needinfo?(jonas)
Flags: needinfo?(amarchesini)

    
    

    
  
My understanding of how userContextIds are intended to work is that we always set the userContextId for a tab before we load anything in it. I'm not sure what the exact UX is, but if the user is able to change context on an existing tab, we should always do that by reloading the toplevel document currently in the tab.

Kate, can you confirm?

In fact, I think we should make SetOriginAttributes assert if the docshell contains anything other than an about:blank document (unfortunately I'm not sure what the best way is to test that).
Flags: needinfo?(jonas) → needinfo?(kmckinley)

    
    

    
  
There is no way for a user to change the context for a specific tab, and a tab should not change its userContextId after it starts loading. If the user wants to change contexts, they can open a new tab.

It is highly likely in e10s that a change in userContextId will cause intermittent crashes.
Flags: needinfo?(kmckinley)

    
    

    
  
> In fact, I think we should make SetOriginAttributes assert if the docshell
> contains anything other than an about:blank document (unfortunately I'm not
> sure what the best way is to test that).

We use setUserContextId() also when we are restoring sessions. In this case the content can be different than about:blank. For instance it can be about:sessionrestore. Hopefully it will be always an about:something page. 

Plus: in the docShell chain, we can have some remote content. This means that the parent docshell is still in the parent process (still using 'content' docshell type) but the child is a remote one. This means that we must propagate the UCI from parent to child.

Plus: the reason why bug 1209162 is doing something wrong, is because we cannot inherit UCI from parent to child without checking if they are of the same type (chrome/content/other). I guess we don't want to use UCI in chrome docShells, but just in content ones.
Flags: needinfo?(amarchesini)

    
    

    
  
I think that we should never change the originAttributes of a docshell when it contains a document. Not even a about:sessionrestore document, ideally not even an about:blank document.

Could you try to add an assertion in SetOriginAttributes which verifies that mContentViewer is null?

    
    

    
  
Thanks for the answer, Jonas, Kate and Baku.

I still have some questions in mind,

(In reply to Andrea Marchesini (:baku) from comment #27)
> We use setUserContextId() also when we are restoring sessions. In this case
> the content can be different than about:blank. For instance it can be
> about:sessionrestore. Hopefully it will be always an about:something page. 
> 
> Plus: in the docShell chain, we can have some remote content. This means
> that the parent docshell is still in the parent process (still using
> 'content' docshell type) but the child is a remote one. This means that we
> must propagate the UCI from parent to child.
> Plus: the reason why bug 1209162 is doing something wrong, is because we
> cannot inherit UCI from parent to child without checking if they are of the
> same type (chrome/content/other). I guess we don't want to use UCI in chrome
> docShells, but just in content ones.

1. Since you mentioned 'propagate the UCI from parent to child (with the same mItemType)', 
so do we need some DocShellOriginAttributes.InheritFromParentDocShell ?
actually I've added this in Bug1165466, Jonas said that child docShell should NOT inherit OA from parent docshell, but from the OA of document from parent docshell.
Therefore we changed it in Bug 1209162

From baku's comment, when child docShell wants to inherit OA, it also needs to look at the itemType of parent docshell, which doesn't exist in the parent's document.
So do we need to add InheritFromParentDocShell back?

2. Setting child docshell's UCI is for something like Session Restore or about::someting, but Jonas said 
(In reply to Jonas Sicking (:sicking) from comment #28)
> I think that we should never change the originAttributes of a docshell when
> it contains a document. Not even a about:sessionrestore document, ideally
> not even an about:blank document.

I am confused that whether we should set child docshell's UCI or not.:P

But if we should,
so my understanding is that in these cases the child docShells have been created already, but didn't load any document yet, is this correct?
So I'd add assert as Jonas requested.

(In reply to Jonas Sicking (:sicking) from comment #28)
> Could you try to add an assertion in SetOriginAttributes which verifies that
> mContentViewer is null?

Thanks

    
    

    
  
> 1. Since you mentioned 'propagate the UCI from parent to child (with the
> same mItemType)', 
> so do we need some DocShellOriginAttributes.InheritFromParentDocShell ?
> actually I've added this in Bug1165466, Jonas said that child docShell
> should NOT inherit OA from parent docshell, but from the OA of document from
> parent docshell.
> Therefore we changed it in Bug 1209162

I think that any time that we set UCI on a docshell, that docshell should definitely not have any child docshells. Ideally it shouldn't even have a document (i.e. mContentViewer should be null).

So the question should ideally be irrelevant since there should be nowhere to propagate the UCI to.

It would be great to add an assertion to verify that that's the case though.

> 2. Setting child docshell's UCI is for something like Session Restore or
> about::someting, but Jonas said 
> (In reply to Jonas Sicking (:sicking) from comment #28)
> > I think that we should never change the originAttributes of a docshell when
> > it contains a document. Not even a about:sessionrestore document, ideally
> > not even an about:blank document.
> 
> I am confused that whether we should set child docshell's UCI or not.:P

See above. I don't think there should ever be a child docshell when SetOriginAttributes is called.

During session restore we should be creating new tabs for any page which has a non-empty UCI.

    
    

    
  
(In reply to Jonas Sicking (:sicking) from comment #23)
> ::: docshell/base/nsIDocShell.idl
> @@ +1077,3 @@
> >     */
> > +  [implicit_jscontext]
> > +  attribute jsval docShellOriginAttributes;
> 
> Can we make this into a getter function and a setter function instead.
> 
> When it's an attribute you'd generally expect that
> 
> docshell.docShellOriginAttributes == docshell.docShellOriginAttributes;
> 
> would return true, but that's not the case since the getter creates a new
> object each time. You'd also expect
> 
> docshell.docShellOriginAttributes.userContextId = x;
> 
> to work.
> 
> If we make this a
> 
> getDocShellOriginAttributes();
> setDocShellOriginAttributes();
> 
> pair of functions instead, it's more clear how they work.
> 

nsIDocShell inherits nsILoadContext, which already has a 'readonly attribute jsval originAttributes;'

So I'll try to add the setter in nsIDocShell first.

    
    

    
  
(In reply to Yoshi Huang[:allstars.chh] from comment #31)
> nsIDocShell inherits nsILoadContext, which already has a 'readonly attribute
> jsval originAttributes;'
> 
> So I'll try to add the setter in nsIDocShell first.
Oh sorry, it's nsDocShell inherits nsILoadContext, not nsIDocShell. I'll stick to your comments.
Summary: Add docShellOriginAttributes in nsIDocShell → Add OriginAttributes getter/setter into nsIDocShell

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch. v3 (obsolete)
        — 
          — Splinter Review
      

    


  
rebase, 
I tried to add assert(!mContentViewer) when calling setOriginAttributes
However it causes many crashes on try, and seems to be related to SessionRestore,

I'll probably file another bug to track this.

        Attachment #8708886 -
        Attachment is obsolete: true

        Attachment #8695762 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8712066 [details] [diff] [review]
Patch. v3

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

r? kanru for the TabChild change, as Jonas pointed out in Comment 23 he is not sure for the change either.
r? baku for the removal of calling child.setUserContextId when setting userContextId.

        Attachment #8712066 -
        Flags: review?(kchen)

        Attachment #8712066 -
        Flags: review?(amarchesini)

    
    

    
  
Comment on attachment 8712066 [details] [diff] [review]
Patch. v3

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

I'm not a docShell peer. Moving the review request to smaug who helped me with SetUserContextId.

::: docshell/base/nsDocShell.cpp
@@ -4010,5 @@
>      return NS_OK;
>    }
>  
>    aChild->SetTreeOwner(mTreeOwner);
> -  childDocShell->SetUserContextId(mUserContextId);

With your patch, how do we propage the UserContextId to the child docShell?

        Attachment #8712066 -
        Flags: review?(amarchesini) → review?(bugs)

    
    

    
  
Comment on attachment 8712066 [details] [diff] [review]
Patch. v3

> nsDocShell::GetAppId(uint32_t* aAppId)
> {
>-  if (mOwnOrContainingAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID) {
>-    *aAppId = mOwnOrContainingAppId;
>+  if (mOriginAttributes.mAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID &&
>+      mOriginAttributes.mAppId != nsIScriptSecurityManager::NO_APP_ID) {
>+    *aAppId = mOriginAttributes.mAppId;
>     return NS_OK;
>   }
Why this change? Feels to me that we're missing to update originAttributes.mAppId somewhere.

>+// Implements nsILoadContext.originAttributes
>+NS_IMETHODIMP
>+nsDocShell::GetOriginAttributes(JS::MutableHandle<JS::Value> aVal)
Could we make nsILoadContext.originAttributes to have also [implicit_jscontext]
and then nsDocShell would have just one implementation for GetOriginAttributes(.

>+  JSContext* cx = nsContentUtils::GetCurrentJSContext();
... since I'd prefer passing cx explicitly and using GetCurrentJSContext() as rarely as possible.

>+nsDocShell::SetOriginAttributes(JS::Handle<JS::Value> aOriginAttributes,
>+                                JSContext* aCx)
> {
>-  JSContext* cx = nsContentUtils::GetCurrentJSContext();
>-  MOZ_ASSERT(cx);
>+  DocShellOriginAttributes attrs;
>+  if (!aOriginAttributes.isObject() || !attrs.Init(aCx, aOriginAttributes)) {
>+    return NS_ERROR_INVALID_ARG;
>+  }
> 
>-  bool ok = ToJSValue(cx, GetOriginAttributes(), aVal);
>-  NS_ENSURE_TRUE(ok, NS_ERROR_FAILURE);
>+  SetOriginAttributes(attrs);
>   return NS_OK;
> }
So we move userContextId to originAttributes. Currently when userContetId is set, it is set to all the child frames too.
Why we don't do that with origin attributes. Feels like a bug to me.
(hmm, perhaps set only same-type child docshell. So, same type and same frametype I guess.)

>@@ -4006,17 +4004,16 @@ nsDocShell::AddChild(nsIDocShellTreeItem* aChild)
>     childDocShell->SetUseGlobalHistory(true);
>   }
> 
>   if (aChild->ItemType() != mItemType) {
>     return NS_OK;
>   }
> 
>   aChild->SetTreeOwner(mTreeOwner);
>-  childDocShell->SetUserContextId(mUserContextId);
Does this break the stuff baku just fixed: we need to inherit user context id also in iframes of some page.

>+++ b/dom/base/nsFrameLoader.cpp
>@@ -1784,36 +1784,16 @@ nsFrameLoader::MaybeCreateDocShell()
>       mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::id, frameName);
>     }
>   }
> 
>   if (!frameName.IsEmpty()) {
>     mDocShell->SetName(frameName);
>   }
> 
>-  //Grab the userContextId from owner if XUL
>-  nsAutoString userContextIdStr;
>-  if (namespaceID == kNameSpaceID_XUL) {
>-    if (mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::usercontextid)) {
>-      mOwnerContent->GetAttr(kNameSpaceID_None,
>-                             nsGkAtoms::usercontextid,
>-                             userContextIdStr);
>-    }
>-  }
>-
>-  if (!userContextIdStr.IsEmpty()) {
>-    nsresult rv;
>-    uint32_t userContextId =
>-      static_cast<uint32_t>(userContextIdStr.ToInteger(&rv));
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    rv = mDocShell->SetUserContextId(userContextId);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-  }
>-
>   // Inform our docShell that it has a new child.
>   // Note: This logic duplicates a lot of logic in
>   // nsSubDocumentFrame::AttributeChanged.  We should fix that.
> 
>   int32_t parentType = docShell->ItemType();
> 
>   // XXXbz why is this in content code, exactly?  We should handle
>   // this some other way.....  Not sure how yet.
>@@ -1874,43 +1854,64 @@ nsFrameLoader::MaybeCreateDocShell()
>     nsCOMPtr<nsISHistory> sessionHistory =
>       do_CreateInstance(NS_SHISTORY_CONTRACTID, &rv);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     nsCOMPtr<nsIWebNavigation> webNav(do_QueryInterface(mDocShell));
>     webNav->SetSessionHistory(sessionHistory);
>   }
> 
>+  DocShellOriginAttributes attrs;
>+  nsCOMPtr<nsIPrincipal> parentPrin = doc->NodePrincipal();
>+  PrincipalOriginAttributes poa = BasePrincipal::Cast(parentPrin)->OriginAttributesRef();
>+  attrs.InheritFromDocToChildDocShell(poa);
>+
>   if (OwnerIsAppFrame()) {
>     // You can't be both an app and a browser frame.
>     MOZ_ASSERT(!OwnerIsBrowserFrame());
> 
>     nsCOMPtr<mozIApplication> ownApp = GetOwnApp();
>     MOZ_ASSERT(ownApp);
>     uint32_t ownAppId = nsIScriptSecurityManager::NO_APP_ID;
>     if (ownApp) {
>       NS_ENSURE_SUCCESS(ownApp->GetLocalId(&ownAppId), NS_ERROR_FAILURE);
>     }
> 
>-    mDocShell->SetIsApp(ownAppId);
>+    attrs.mAppId = ownAppId;
>+    mDocShell->SetFrameType(nsIDocShell::FRAME_TYPE_APP);
>   }
> 
>   if (OwnerIsBrowserFrame()) {
>     // You can't be both a browser and an app frame.
>     MOZ_ASSERT(!OwnerIsAppFrame());
> 
>     nsCOMPtr<mozIApplication> containingApp = GetContainingApp();
>     uint32_t containingAppId = nsIScriptSecurityManager::NO_APP_ID;
>     if (containingApp) {
>       NS_ENSURE_SUCCESS(containingApp->GetLocalId(&containingAppId),
>                         NS_ERROR_FAILURE);
>     }
>-    mDocShell->SetIsBrowserInsideApp(containingAppId);
>+
>+    attrs.mAppId = containingAppId;
>+    attrs.mInBrowser = true;
>+    mDocShell->SetFrameType(nsIDocShell::FRAME_TYPE_BROWSER);
Tiny bit annoying to move this kind stuff outside docshell. Is there any chance to keep this logic in DocShell?

>+  // Grab the userContextId from owner if XUL
>+  nsAutoString userContextIdStr;
>+  if ((namespaceID == kNameSpaceID_XUL) &&
>+      mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::usercontextid, userContextIdStr) &&
>+      !userContextIdStr.IsEmpty()) {
>+    nsresult rv;
>+    attrs.mUserContextId = userContextIdStr.ToInteger(&rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>   }
> 
>+  nsDocShell::Cast(mDocShell)->SetOriginAttributes(attrs);
So here tabs inherit whatever origin attributes chrome docshell has (in case of non-e10s), not
only iframe inheriting from their parent document.
We really need to not inherit random stuff. Also this is something which should be, IMO, in docshell.
nsDocShell::AddChild could do the inheritance stuff.

        Attachment #8712066 -
        Flags: review?(bugs) → review-

    
    

    
  
> Tiny bit annoying to move this kind stuff outside docshell. Is there any chance to keep this logic in
> DocShell?

Actually, the place where I'd like this logic to end up is in DocShellOriginAttributes::InheritFromDocToChildDocShell. That function should eventually take just an nsIContent* which is the <iframe>/<xul:browser>/<frame> element that we're inheriting from.

That'll follow the pattern of other *OriginAttributes::InheritFrom* functions.

But I think we should do that as a followup.

    
    

    
  
(In reply to Andrea Marchesini (:baku) from comment #37)
> With your patch, how do we propage the UserContextId to the child docShell?

There really should never be any child docshells when we set the OriginAttributes on a docshell. If there is that would indicate that we have a "real" document in the docshell, and we can't mutate the OriginAttributes of a running document. That would be like changing the principal of a running document.

    
    

    
  
DocShellOriginAttributes sounds good too. Certainly better than moving more and more stuff to
nsFrameLoader. 
Though, DocShellOriginAttributes itself can't set stuff like docshell mFrameType.

    
    

    
  
I don't really understand the frametype business. So far we're just keeping it as-is other than shuffling how it gets set. I agree that it's unfortunate to have to set it separately, but it's better than what we have now.

But maybe we could set the frametime in SetOriginAttributes? I had not thought about that possibility. I don't know if that is possible?

    
    

    
  
(In reply to Jonas Sicking (:sicking) from comment #40)
> (In reply to Andrea Marchesini (:baku) from comment #37)
> > With your patch, how do we propage the UserContextId to the child docShell?
> 
> There really should never be any child docshells when we set the
> OriginAttributes on a docshell.
baku's comment is about the case when we add new iframes to a document. And yes, the patch does seem to inherit origin attributes from parent document in nsFrameLoader (but it inherits also random bits from chrome docshell).
The inheritance should be handled somewhere in docshell, IMO.

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #43)
> (In reply to Jonas Sicking (:sicking) from comment #40)
> > (In reply to Andrea Marchesini (:baku) from comment #37)
> > > With your patch, how do we propage the UserContextId to the child docShell?
> > 
> > There really should never be any child docshells when we set the
> > OriginAttributes on a docshell.
> baku's comment is about the case when we add new iframes to a document. And
> yes, the patch does seem to inherit origin attributes from parent document
> in nsFrameLoader (but it inherits also random bits from chrome docshell).

I'd be fine with making the inheriting code explicitly forbidding anything from being inherited from chrome document to any contained content docshells. Fhat seems to do the right thing for all properties that we have in OriginAttributes for now.

If we eventually move private browsing to become an OriginAttribute (which I think would simplify a lot of things), then it might make sense to have the private-browsing flag be set on the chrome window and then inherit into the content docshells. I'm not sure. We can solve that when we get there.

> The inheritance should be handled somewhere in docshell, IMO.

Happy to let you decide where what code should go. The only thing I would say is that this bug is blocking a lot of other work and I think already significantly improves the flow of OriginAttributes. So it might be good to improve things in stages.

Up to you.

    
    

    
  
Thanks for the review, smaug.

(In reply to Olli Pettay [:smaug] from comment #38)
> Comment on attachment 8712066 [details] [diff] [review]
> Patch. v3
> 
> > nsDocShell::GetAppId(uint32_t* aAppId)
> > {
> >-  if (mOwnOrContainingAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID) {
> >-    *aAppId = mOwnOrContainingAppId;
> >+  if (mOriginAttributes.mAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID &&
> >+      mOriginAttributes.mAppId != nsIScriptSecurityManager::NO_APP_ID) {
> >+    *aAppId = mOriginAttributes.mAppId;
> >     return NS_OK;
> >   }
> Why this change? Feels to me that we're missing to update
> originAttributes.mAppId somewhere.
>
Originally mOwnOrContainingAppId is initialized to UNKNOWN_APP_ID, now we use origin attributes, and the default value of appId in OriginAttributes is set to NO_APP_ID, so I added the check for NO_APP_ID.

> >+// Implements nsILoadContext.originAttributes
> >+NS_IMETHODIMP
> >+nsDocShell::GetOriginAttributes(JS::MutableHandle<JS::Value> aVal)
> Could we make nsILoadContext.originAttributes to have also
> [implicit_jscontext]
> and then nsDocShell would have just one implementation for
> GetOriginAttributes(.
> 
Will do.


> >+nsDocShell::SetOriginAttributes(JS::Handle<JS::Value> aOriginAttributes,
> >+                                JSContext* aCx)
> > {
> >-  JSContext* cx = nsContentUtils::GetCurrentJSContext();
> >-  MOZ_ASSERT(cx);
> >+  DocShellOriginAttributes attrs;
> >+  if (!aOriginAttributes.isObject() || !attrs.Init(aCx, aOriginAttributes)) {
> >+    return NS_ERROR_INVALID_ARG;
> >+  }
> > 
> >-  bool ok = ToJSValue(cx, GetOriginAttributes(), aVal);
> >-  NS_ENSURE_TRUE(ok, NS_ERROR_FAILURE);
> >+  SetOriginAttributes(attrs);
> >   return NS_OK;
> > }
> So we move userContextId to originAttributes. Currently when userContetId is
> set, it is set to all the child frames too.
> Why we don't do that with origin attributes. Feels like a bug to me.

After reading your Comment 43, 
My understanding is that when a child docshell is added onto a parent docshell, inheritance should propogate the OA, this is what my patch is doing now. The question is *where* we should do the inheritance, as you asked in the end of Comment 43.
But what I am still not sure is should we set origin attributes to the child docshell when we call origin attributes on the *parent* docshell, can you confirm?
Since Jonas says there shouldn't be any child docshell existing when we call setOriginAttributes.

> 
> >@@ -4006,17 +4004,16 @@ nsDocShell::AddChild(nsIDocShellTreeItem* aChild)
> >     childDocShell->SetUseGlobalHistory(true);
> >   }
> > 
> >   if (aChild->ItemType() != mItemType) {
> >     return NS_OK;
> >   }
> > 
> >   aChild->SetTreeOwner(mTreeOwner);
> >-  childDocShell->SetUserContextId(mUserContextId);
> Does this break the stuff baku just fixed: we need to inherit user context
> id also in iframes of some page.
> 
See my comment above.

> >+
> >+    attrs.mAppId = containingAppId;
> >+    attrs.mInBrowser = true;
> >+    mDocShell->SetFrameType(nsIDocShell::FRAME_TYPE_BROWSER);
> Tiny bit annoying to move this kind stuff outside docshell. Is there any
> chance to keep this logic in DocShell?
>
As Jonas mentioned in comment 39, I'll file a follow-up.

> >+  // Grab the userContextId from owner if XUL
> >+  nsAutoString userContextIdStr;
> >+  if ((namespaceID == kNameSpaceID_XUL) &&
> >+      mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::usercontextid, userContextIdStr) &&
> >+      !userContextIdStr.IsEmpty()) {
> >+    nsresult rv;
> >+    attrs.mUserContextId = userContextIdStr.ToInteger(&rv);
> >+    NS_ENSURE_SUCCESS(rv, rv);
> >   }
> > 
> >+  nsDocShell::Cast(mDocShell)->SetOriginAttributes(attrs);
> So here tabs inherit whatever origin attributes chrome docshell has (in case
> of non-e10s), not
> only iframe inheriting from their parent document.
This is a good point, I am not sure about chrome docshell.
I'll check this, and decide whether I'll fix it here or another bug for this.

> We really need to not inherit random stuff.
There are already a couple bugs for inheritance, see the meta bug Bug 1153435.
Or could you be more specific and share your thoughts here?


> Also this is something which
> should be, IMO, in docshell.
> nsDocShell::AddChild could do the inheritance stuff.
Good suggestion, so I'll move the ineheritance inside nsDocShell::AddChild, instead of nsFrameLoader:MaybeCreateDocShell so far.

Thanks

    
    

    
  
(In reply to Jonas Sicking (:sicking) from comment #44)
> (In reply to Olli Pettay [:smaug] from comment #43)
> > (In reply to Jonas Sicking (:sicking) from comment #40)
> > > (In reply to Andrea Marchesini (:baku) from comment #37)
> > > > With your patch, how do we propage the UserContextId to the child docShell?
> > > 
> > > There really should never be any child docshells when we set the
> > > OriginAttributes on a docshell.
> > baku's comment is about the case when we add new iframes to a document. And
> > yes, the patch does seem to inherit origin attributes from parent document
> > in nsFrameLoader (but it inherits also random bits from chrome docshell).
> 
> I'd be fine with making the inheriting code explicitly forbidding anything
> from being inherited from chrome document to any contained content
> docshells. Fhat seems to do the right thing for all properties that we have
> in OriginAttributes for now.

The userContextId is currently inherited from chrome since it gets set in tabbrowser.xml at tab creation on the tab when you do File->New Contextual Identity Tab. But once set, it should not need to be done again.

> 
> If we eventually move private browsing to become an OriginAttribute (which I
> think would simplify a lot of things), then it might make sense to have the
> private-browsing flag be set on the chrome window and then inherit into the
> content docshells. I'm not sure. We can solve that when we get there.

In this case, it would be no different from how userContextId works now.

    
    

    
  
(In reply to Kate McKinley [:kmckinley] from comment #46)
> The userContextId is currently inherited from chrome since it gets set in
> tabbrowser.xml at tab creation on the tab when you do File->New Contextual
> Identity Tab. But once set, it should not need to be done again.
> 
It is not really inherited. It is set in tabbrowser.xml, but not 
inherited from chrome docshell.

    
    

    
  
(In reply to Yoshi Huang[:allstars.chh] from comment #45)
> > Also this is something which
> > should be, IMO, in docshell.
> > nsDocShell::AddChild could do the inheritance stuff.
>
> Good suggestion, so I'll move the ineheritance inside nsDocShell::AddChild,
> instead of nsFrameLoader:MaybeCreateDocShell so far.

I don't think we can only add this to nsDocShell::AddChild. I don't think that would work for e10s.

Currently the way things work is that nsFrameLoader:MaybeCreateDocShell gathers information into an DocShellOriginAttributes and then sets that either on the child docshell, or the TabParent in which we will eventually create a inner docshell.

We'll need something like that in the future as well.

One thing to keep in mind here is that we need the parent process to safely store at least some of the origin attributes so that we can get dependable information even if the child process is hacked. For example the signed package ID is something we need to store in the parent process's TabParent. The best way to do this is likely to store a DocShellOriginAttributes in the parent, and then run code similar to [1] to verify that the child process isn't lying.

In short, I think we should keep storing a DocShellOriginAttributes in the TabParent. And we need to set the information there without going through the child process docshell.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/AppProcessChecker.cpp#242

    
    

    
  
Comment on attachment 8712066 [details] [diff] [review]
Patch. v3

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

Only the TabChild part.

        Attachment #8712066 -
        Flags: review?(kchen) → review+

    
    

    
  
(In reply to Yoshi Huang[:allstars.chh] from comment #45)
> 
> > >+// Implements nsILoadContext.originAttributes
> > >+NS_IMETHODIMP
> > >+nsDocShell::GetOriginAttributes(JS::MutableHandle<JS::Value> aVal)
> > Could we make nsILoadContext.originAttributes to have also
> > [implicit_jscontext]
> > and then nsDocShell would have just one implementation for
> > GetOriginAttributes(.
> > 
> Will do.

Oh, no
I remember now why I didn't add [implicit_jscontext] in nsILoadContext now,
"IDL methods marked with [implicit_jscontext] or [optional_argc] may not be implemented in JS"

    
    

    
  
ni? for smaug for Comment 45,
"But what I am still not sure is should we set origin attributes to the child docshell when we call origin attributes on the *parent* docshell, can you confirm?
Since Jonas says there shouldn't be any child docshell existing when we call setOriginAttributes."

I'd really like to get an agreement on this, but it looks to me that Jonas says "We shouldn't", and smaug and baku say "We should".

And also check Jonas' comment 48 for the AddChild explanation.

Thanks
Flags: needinfo?(bugs)

    
    

    
  
There absolutely should not be any child docshells when SetOriginAttributes is called. If there is that is a bug.

Can you add an assertion and see if we hit it? If the assertion is not triggered, then I am right :)

In fact, there really shouldn't be any other document then an about:blank document in the docshell when SetOriginAttributes is called. If there is then that's a very serious security bug.

We should, in a followup bug, see if we can make it possible to assert that when SetOriginAttributes is called, that mContentViewer is null, which means that there is no document at all (and hence can't be any child docshells).

    
    

    
  
(In reply to Jonas Sicking (:sicking) from comment #53)
> There absolutely should not be any child docshells when SetOriginAttributes
> is called. If there is that is a bug.
> 
> Can you add an assertion and see if we hit it? If the assertion is not
> triggered, then I am right :)
> 
> In fact, there really shouldn't be any other document then an about:blank
> document in the docshell when SetOriginAttributes is called. If there is
> then that's a very serious security bug.
> 
Yeah, you're right
I've tried some tests, and all of them doesn't have children when setOriginAttributes is called.
And the doc that mContentViewer holds is about:blank

> We should, in a followup bug, see if we can make it possible to assert that
> when SetOriginAttributes is called, that mContentViewer is null, which means
> that there is no document at all (and hence can't be any child docshells).
Will do,

I'll run a full try to get a more accurate result.

Thanks

    
    

    
  


  
This is what Jonas reviewed before.
I moved the assertion to Part 2.

        Attachment #8712066 -
        Attachment is obsolete: true

        Attachment #8713548 -
        Attachment description: Part 1. v4 → Part 1. add originAttributes into nsIDocShell. v4

    
    

    
  
(In reply to Jonas Sicking (:sicking) from comment #53)
> There absolutely should not be any child docshells when SetOriginAttributes
> is called. If there is that is a bug.

or feature. Depends on how we want this all work.

> 
> Can you add an assertion and see if we hit it? If the assertion is not
> triggered, then I am right :)

But yes, I'd be ok to add just an assertion that when origin attributes are being set, docshell must 
not have any child shells.
Flags: needinfo?(bugs)

    
    

    
  
The OriginAttributes of a Document is a function of the OriginAttributes of the Document's docshell. So if the OriginAttributes of the docshell changes, that can well mean that the OriginAttributes of the Document should change. But the OriginAttributes of a Document is part of the Document's origin and the documents principal. It affects things like permissions, cookies, and indexedDB data that is available to that Document.

I feel strongly that we don't want the principal of a running document to change such that it affects the permissions, cookies and indexedDB data available to that document. The fact that the document.domain of a document can change is enough of a problem already, and this would be significantly worse.

So I definitely think that changing the OriginAttributes of a docshell after it has started being used is a bug. That was certainly the intent that bholley and I had when designing OriginAttributes.

It actually worries me enough that we've instantiated the about:blank document by the time that SetOriginAttributes is called. I'd really like to see that fixed. But in a separate bug likely.

    
    

    
  
I'm totally fine with allowing SetOriginAttributes call only when there is about:blank and not child shells.
And if you're worried about about:blank, we could try to push SetOriginAttributes handling to happen right after docshell is created, not some random time later. The hard part is session restore, but even that should be doable. Like we could replace the existing docshell when setting originattributes on a frameloader or tabchild or something. (though better would be to know originattributes before frameloader or tabchild is created)


(and sorry, I don't know all the background for originattributes, that stuff just started to show up at some point  ;) )

    
    

    
  
I've got a full try result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=45a8004b0f04&selectedJob=16087995

Some tests fail in the assertion of "about:blank", I didn't figure out the root cause yet, but mostly the case is when loading another URI, gBrowser still keeps the previous URI, so when SessionStore.jsm is going to recover the session it will triggger the assert.

I'll file a follow-up for checking this.

    
    

    
  
now assert on the no child docshell will also have try error
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dafbc43549ab&selectedJob=16155906

So I'll move Part 2 as a seperate bug.

    
    

    
  
Comment on attachment 8713548 [details] [diff] [review]
Part 1. add originAttributes into nsIDocShell. v4

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

Hi smaug
This is the same version you reviewed last time,
and I've answered some of your comment in Comment 45, and Comment 51.
As Jonas mentioned AddChild shouldn't do the inheritance code so I didn't change it in the end.
For adding the assert (Part 2) now will cause some failures on try, therefore I'll move it to a seperate bug.

Thanks

        Attachment #8713548 -
        Flags: review?(bugs)

    
    

    
  
Comment on attachment 8713548 [details] [diff] [review]
Part 1. add originAttributes into nsIDocShell. v4

>+// Implements nsILoadContext.originAttributes
>+NS_IMETHODIMP
>+nsDocShell::GetOriginAttributes(JS::MutableHandle<JS::Value> aVal)
So couldn't this take a cx as param? Make the .idl to use implicit_context or so


>@@ -1874,43 +1854,64 @@ nsFrameLoader::MaybeCreateDocShell()
>     nsCOMPtr<nsISHistory> sessionHistory =
>       do_CreateInstance(NS_SHISTORY_CONTRACTID, &rv);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     nsCOMPtr<nsIWebNavigation> webNav(do_QueryInterface(mDocShell));
>     webNav->SetSessionHistory(sessionHistory);
>   }
> 
>+  DocShellOriginAttributes attrs;
>+  nsCOMPtr<nsIPrincipal> parentPrin = doc->NodePrincipal();
>+  PrincipalOriginAttributes poa = BasePrincipal::Cast(parentPrin)->OriginAttributesRef();
>+  attrs.InheritFromDocToChildDocShell(poa);
So I still don't like this. We inherit random stuff from chrome docshell to content docshell.
And I'm not talking about taking attributes from xul:browser, but whatever is defined in chrome document's principal.
We sure need to check the type of the docshell, and possibly also the app type before inheriting anything?
(it is not clear to me whether we want to inherit all of origin attributes in case the app id changes)
So, we need a whitelist here and inherit from doc's principal only in the cases we really want, not always.
Remember, this same code is called for <xul:browser type="content"> containing web pages and <html:iframe> inside web pages.

(Random wondering. Why does nsFrameLoader::GetNewTabContext always use usercontextid. Do we let apps to set that on
html:iframe too? But non-OOP case does check that the element is in xul namespace)

        Attachment #8713548 -
        Flags: review?(bugs) → review-

    
    

    
  
(In reply to Olli Pettay [:smaug] (high review load) from comment #66)
> Comment on attachment 8713548 [details] [diff] [review]
> Part 1. add originAttributes into nsIDocShell. v4
> 
> >+// Implements nsILoadContext.originAttributes
> >+NS_IMETHODIMP
> >+nsDocShell::GetOriginAttributes(JS::MutableHandle<JS::Value> aVal)
> So couldn't this take a cx as param? Make the .idl to use implicit_context
> or so
> 
Comment 51

> 
> >@@ -1874,43 +1854,64 @@ nsFrameLoader::MaybeCreateDocShell()
> >     nsCOMPtr<nsISHistory> sessionHistory =
> >       do_CreateInstance(NS_SHISTORY_CONTRACTID, &rv);
> >     NS_ENSURE_SUCCESS(rv, rv);
> > 
> >     nsCOMPtr<nsIWebNavigation> webNav(do_QueryInterface(mDocShell));
> >     webNav->SetSessionHistory(sessionHistory);
> >   }
> > 
> >+  DocShellOriginAttributes attrs;
> >+  nsCOMPtr<nsIPrincipal> parentPrin = doc->NodePrincipal();
> >+  PrincipalOriginAttributes poa = BasePrincipal::Cast(parentPrin)->OriginAttributesRef();
> >+  attrs.InheritFromDocToChildDocShell(poa);
> So I still don't like this. We inherit random stuff from chrome docshell to
> content docshell.
> And I'm not talking about taking attributes from xul:browser, but whatever
> is defined in chrome document's principal.
> We sure need to check the type of the docshell, and possibly also the app
> type before inheriting anything?
> (it is not clear to me whether we want to inherit all of origin attributes
> in case the app id changes)
> So, we need a whitelist here and inherit from doc's principal only in the
> cases we really want, not always.
> Remember, this same code is called for <xul:browser type="content">
> containing web pages and <html:iframe> inside web pages.
> 
So I'll do Jonas' comment 39 in this bug, to add a another nsIContent into InheritFromDocToChildDocShell, so we decide whether we could propogate those attributes in that function, does this sound okay to you ?

Thanks

    
    

    
  


  

        Attachment #8713548 -
        Attachment is obsolete: true

        Attachment #8713550 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8720218 [details] [diff] [review]
Part 1: add nsIContent to InheritFromDocToChildDocShell

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

Hi Jonas
I add this patch for smaug's comment 66, since this is your idea in the first place,
can you help to give me some feedback about this?

So far I try to prevent copying OriginAttributes from xul:browser as smuag mentioned, I guess we should also check it's chrome or content docshell, but I am not sure how to use nsIContent to check this.

Can you give me some suggestions here

Thanks

        Attachment #8720218 -
        Flags: feedback?(jonas)

    
    

    
  
(In reply to Yoshi Huang[:allstars.chh] from comment #68)
> So I'll do Jonas' comment 39 in this bug, to add a another nsIContent into
> InheritFromDocToChildDocShell, so we decide whether we could propogate those
> attributes in that function, does this sound okay to you ?
yeah, that approach should be fine. Hopefully InheritFromDocToChildDocShell is the one place deciding what all gets
inherited.

    
    

    
  
Comment on attachment 8720218 [details] [diff] [review]
Part 1: add nsIContent to InheritFromDocToChildDocShell

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

I think we should keep things simple in order to move things forward here.

Rather than add the nsIContent argument to DocShellOriginAttributes::InheritFromDocToChildDocShell, just make nsFrameLoader::MaybeCreateDocShell not call DocShellOriginAttributes::InheritFromDocToChildDocShell if the element is a xul:browser element.

Then, in a separate bug, we should add the nsIContent argument, but we should also move a lot more handling from nsFrameLoader::MaybeCreateDocShell to DocShellOriginAttributes::InheritFromDocToChildDocShell.

Specifically what DocShellOriginAttributes::InheritFromDocToChildDocShell should do is:
* Check if it's a xul:browser element *and* the element has type=content (or whatever syntax it is that we use to create content docshells), and if so set the OA to its default values, rather than inherit from the nsIContent.
* Check for usercontextid attributes. This should be done even if the nsIContent is a xul:browser and/or has type=content.
* Check for mozapp and mozbrowser attributes. This should only be done if the nsIContent is a html:iframe or a html:frame.
* Maybe other things that I'm missing.

All of this is clearly too much to do in this bug. Which is why I would encourage us to do it later.

For now, I would say, just make nsFrameLoader::MaybeCreateDocShell not call DocShellOriginAttributes::InheritFromDocToChildDocShell if the element is a xul:browser.

        Attachment #8720218 -
        Flags: feedback?(jonas) → feedback-

    
    

    
  


  
added IsXUL check in nsFrameLoader.cpp

        Attachment #8720218 -
        Attachment is obsolete: true

        Attachment #8720219 -
        Attachment is obsolete: true

        Attachment #8721227 -
        Flags: review?(jonas)

        Attachment #8721227 -
        Flags: review?(bugs)

    
    

    
  
(In reply to Yoshi Huang[:allstars.chh] from comment #51)
> Oh, no
> I remember now why I didn't add [implicit_jscontext] in nsILoadContext now,
> "IDL methods marked with [implicit_jscontext] or [optional_argc] may not be
> implemented in JS"
So where do we implement nsILoadContext in JS?

    
    

    
  
(In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #77)
> (In reply to Yoshi Huang[:allstars.chh] from comment #51)
> > Oh, no
> > I remember now why I didn't add [implicit_jscontext] in nsILoadContext now,
> > "IDL methods marked with [implicit_jscontext] or [optional_argc] may not be
> > implemented in JS"
> So where do we implement nsILoadContext in JS?

Mostly in tests written in JS from Necko, see https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1165466&attachment=8665349, those are the tests implementing nsILoadContext in JS.

    
    

    
  
Seems silly to keep an interface scriptable just for tests. However more and more other work is being held up by this patch landing, so I'd rather not block this on rewriting tests. I'd rather consider that for a followup bug.

    
    

    
  
(In reply to Yoshi Huang[:allstars.chh] from comment #45)
> Thanks for the review, smaug.
> 
> (In reply to Olli Pettay [:smaug] from comment #38)
> > Comment on attachment 8712066 [details] [diff] [review]
> > Patch. v3
> > 
> > > nsDocShell::GetAppId(uint32_t* aAppId)
> > > {
> > >-  if (mOwnOrContainingAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID) {
> > >-    *aAppId = mOwnOrContainingAppId;
> > >+  if (mOriginAttributes.mAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID &&
> > >+      mOriginAttributes.mAppId != nsIScriptSecurityManager::NO_APP_ID) {
> > >+    *aAppId = mOriginAttributes.mAppId;
> > >     return NS_OK;
> > >   }
> > Why this change? Feels to me that we're missing to update
> > originAttributes.mAppId somewhere.
> >
> Originally mOwnOrContainingAppId is initialized to UNKNOWN_APP_ID, now we
> use origin attributes, and the default value of appId in OriginAttributes is
> set to NO_APP_ID, so I added the check for NO_APP_ID.

I don't still understand the change. nsFrameLoader updates appid anyhow, and so should TabChild.
So I don't understand the need for the change.

    
    

    
  
Comment on attachment 8721227 [details] [diff] [review]
add origin attributes into nsIDocShell. v6

So please explain why that NO_APP_ID check is needed. And please coordinate this stuff with bug 1238160, since

+  DocShellOriginAttributes attrs;
+
+  if (!mOwnerContent->IsXULElement(nsGkAtoms::browser)) {
+    nsCOMPtr<nsIPrincipal> parentPrin = doc->NodePrincipal();
+    PrincipalOriginAttributes poa = BasePrincipal::Cast(parentPrin)->OriginAttributesRef();
+    attrs.InheritFromDocToChildDocShell(poa);
+  }
won't work there.
We really need to check the type of docshells, not element type.
But that change needs to happen just when bug 1238160 lands.

        Attachment #8721227 -
        Flags: review?(bugs) → review+

    
    

    
  
(In reply to Olli Pettay [:smaug] (vacation Feb 29 - Mar 4) from comment #80)
> > >
> > Originally mOwnOrContainingAppId is initialized to UNKNOWN_APP_ID, now we
> > use origin attributes, and the default value of appId in OriginAttributes is
> > set to NO_APP_ID, so I added the check for NO_APP_ID.
> 
> I don't still understand the change. nsFrameLoader updates appid anyhow, and
> so should TabChild.
> So I don't understand the need for the change.

okay, make sense. I'll revert the line back.
Thanks for pointing this out.

(In reply to Olli Pettay [:smaug] (vacation Feb 29 - Mar 4) from comment #81)
> won't work there.
> We really need to check the type of docshells, not element type.
> But that change needs to happen just when bug 1238160 lands.
Okay, I'll wait and rebase once it is landed.

    
    

    
  
> Okay, I'll wait and rebase once it is landed.

No, we can not block on that bug. As I've mentioned several times, the fact that this bug has not yet landed is already holding up other things.

I feel like we are going around in circles. And it's made worse by timezones getting in our way. I suggest that we get the three of us (Yoshi, Smaug and me) on a vidyo call and hammer out the remaining issues. I'm happy to be the one staying up late. I'll send you guys an email with a suggested time.

    
    

    
  
I would like to be in this meeting.
Flags: needinfo?(jonas)

    
    

    
  
Comment on attachment 8721227 [details] [diff] [review]
add origin attributes into nsIDocShell. v6

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

r=me.

There's definitely more cleanup that we should do after this lands. I'm very curious why we need the whole bunch of functions for various forms of getting appid and frametype. Hopefully we should be able to simplify that significantly. But let's worry about that in separate bugs.

::: docshell/base/nsDocShell.cpp
@@ +13894,5 @@
>  nsDocShell::GetAppId(uint32_t* aAppId)
>  {
> +  if (mOriginAttributes.mAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID &&
> +      mOriginAttributes.mAppId != nsIScriptSecurityManager::NO_APP_ID) {
> +    *aAppId = mOriginAttributes.mAppId;

This part I am not sure about. Sounds like Smaug says that we should revert this which is fine with me.

        Attachment #8721227 -
        Flags: review?(jonas) → review+

    
    

    
  


  
revert appId change as mentioned by smaug.

        Attachment #8721227 -
        Attachment is obsolete: true

        Attachment #8725517 -
        Flags: review+

    
    

    
  
(In reply to Jonas Sicking (:sicking) from comment #84)
> > Okay, I'll wait and rebase once it is landed.
> 
> No, we can not block on that bug. As I've mentioned several times, the fact
> that this bug has not yet landed is already holding up other things.
> 
The latest patch from Bug 1238160 still has some assertions failures, in that case I will land my patches later today if jryans doesn't upload new patches.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/6588774cd859
Status: NEW → RESOLVED
Closed: 8 years ago

          status-firefox47:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: