e10s: new security API: replace SerializedLoadContext with LoadInfo - Check if they match

RESOLVED FIXED in Firefox 48

Status

()

defect
P3
normal
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: jduell.mcbugs, Assigned: dragana)

Tracking

unspecified
mozilla48
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.5+, e10s+, firefox48 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(5 attachments, 17 obsolete attachments)

4.23 KB, patch
Details | Diff | Splinter Review
5.58 KB, patch
dragana
: review+
Details | Diff | Splinter Review
1.93 KB, patch
Details | Diff | Splinter Review
3.95 KB, patch
dragana
: review+
Details | Diff | Splinter Review
23.72 KB, patch
dragana
: review+
Details | Diff | Splinter Review
Reporter

Description

5 years ago
The new new better way to handle getting the appID from the child (and verifying on the parent) is to pass an nsIPrincipal in IPDL (instead of a SerializedLoadContext) on the child, and on the parent we can now use AssertAppPrincipal() instead of the code at NeckoParent::GetValidatedAppInfo().  (Note: we'll still want to call  if UsingNeckoIPCSecurity() before we verify the appInfo, or we'll break xpcshell tests, so maybe we should keep NeckoParent::GetValidatedAppInfo() and have it just check UsingNeckoSecurity, and if so, call AssertAppPrincipal).

We also need to figure out if we should keep storing appId, inBrowser, etc in a stub LoadContext in the parent channel, or if we should now keep track of appId via the Principal or nsLoadInfo or something.  I'll ask jonas.
Reporter

Comment 1

5 years ago
I'm going to ask in the next Better Necko API meeting what exactly we should do here with regard to nsILoadContext vs other interfaces for storing/fetching appID.  Right now all the necko cookies/localstorage/etc-jar code uses nsILoadContext to get appId, but that could be changed easily enough.
Assignee

Updated

5 years ago
Status: NEW → ASSIGNED
My hope is to transition to getting the appid/browserflag/privatebrowsing info from the nsILoadInfo. The appid and browserflag we can get from the loadingPrincipal.

Private browsing information we could easily store in the nsILoadInfo. The loadinfo constructor already has access to enough information to figure that out. Potentially we could also add privatebrowsing info to nsIPrincipal, but that's probably a bigger project.

For now we can probably start asserting that the appid/browserflag/privatebrowsing info from the nsILoadInfo matches that of the nsILoadContext. I would expect that to find some bugs that we need to fix before we can stop using nsILoadContext for this.
Assignee

Comment 3

4 years ago
I will add the check whether appid, inBrowser flag and private browsing match.

Can you help how to get private browsing information in LoadInfo?
I think the best way is to do something like:

loadingNode->OwnerDoc()->GetLoadContext()->GetUsePrivateBrowsing();

Adding nullchecks where appropriate of course.

It would probably also be useful to add a SEC_FORCE_PRIVATE_BROWSING security flag to nsILoadInfo so that chrome code can force private browsing requests even when it's not happening in the context of a document. I.e. even when loadingNode is null.
Assignee

Comment 5

4 years ago
Posted patch bug_1125916_v1.patch (obsolete) — Splinter Review
This patch only adds SEC_FORCE_PRIVATE_BROWSING and check in NeckoParent whether LoadContext flags are the same as LoadInfo. The checks are not MOZ_ASSERT but it is not possible to create a channel if info does not match.

If SEC_FORCE_PRIVATE_BROWSING is set it will force private browsing even if loadingNode is present.
Attachment #8558509 - Flags: feedback?(jonas)
Attachment #8558509 - Flags: feedback?(jduell.mcbugs)
Comment on attachment 8558509 [details] [diff] [review]
bug_1125916_v1.patch

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

Just some drive by notes; one thing that is of importance is, that we can't serialize the loadContext between child and parent in e10s. Can you please also add the boolean for private browsing to the private constructor [1]? You would then have to query the boolean for private browsing before you pass the information from the child to the parent and call that private (friend) constructor of LoadInfo. I think you can basically do the same thing as we do for the window id, see the patch here [2].

Thanks for fixing this and let me know if you have any questions.

[1] http://mxr.mozilla.org/mozilla-central/source/docshell/base/LoadInfo.h#46
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1107638

::: docshell/base/LoadInfo.cpp
@@ +62,1 @@
>  }

This looks good, but I guess you can slightly simplify it; If I am not mistaken, I think OwnerDoc() never returns null, so the following should work:

  if (aLoadingContext) {
    nsCOMPtr<nsILoadContext> loadContext = aLoadingContext->OwnerDoc()->GetLoadContext();
    if (loadContext) {
      bool usePrivateBrowsing =
        (NS_SUCCEEDED(loadContext->GetUsePrivateBrowsing(&usePrivateBrowsing)) && usePrivateBrowsing);
      if (usePrivateBrowsing) {
        mSecurityFlags |= nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING;
      }
    }
  }
Attachment #8558509 - Flags: feedback+
Assignee

Comment 7

4 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> Comment on attachment 8558509 [details] [diff] [review]
> bug_1125916_v1.patch
> 
> Review of attachment 8558509 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just some drive by notes; one thing that is of importance is, that we can't
> serialize the loadContext between child and parent in e10s. Can you please
> also add the boolean for private browsing to the private constructor [1]?
> You would then have to query the boolean for private browsing before you
> pass the information from the child to the parent and call that private
> (friend) constructor of LoadInfo. I think you can basically do the same
> thing as we do for the window id, see the patch here [2].
> 
> Thanks for fixing this and let me know if you have any questions.
> 
> [1] http://mxr.mozilla.org/mozilla-central/source/docshell/base/LoadInfo.h#46
> [2] https://bugzilla.mozilla.org/show_bug.cgi?id=1107638
> 

There was one think that i was not sure: to have a bool variable mPrivateBrowsing or to store this information into security flags. I went for the second one and in that way we cannot distinguish between  SEC_FORCE_PRIVATE_BROWSING flas set and private browsing set according to nsINode. I do not serialize loadContext and i do not need to. Maybe I misunderstood, now i am querying only securityFlags, but if we need to distinguish between two cases above, i will change it.
(In reply to Dragana Damjanovic [:dragana] from comment #7)
> There was one think that i was not sure: to have a bool variable
> mPrivateBrowsing or to store this information into security flags.

What you do seems correct to me, no need for an additional bool member, just merge the SEC_FORCE_PRIVATE_BROWSING bits into mSecurityFlags.

> I went
> for the second one and in that way we cannot distinguish between 
> SEC_FORCE_PRIVATE_BROWSING flas set and private browsing set according to
> nsINode. I do not serialize loadContext and i do not need to. Maybe I
> misunderstood, now i am querying only securityFlags, but if we need to
> distinguish between two cases above, i will change it.

No, you are right. I was biased by the innerWindowId patch, which works slightly different. In your case the force private browsing bit is already set in the securityflags which we correctly serialize. Sorry about the confusion.
Comment on attachment 8558509 [details] [diff] [review]
bug_1125916_v1.patch

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

::: docshell/base/LoadInfo.cpp
@@ +46,5 @@
> +
> +  if (!(mSecurityFlags & nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING)) {
> +    if (aLoadingContext) {
> +      nsIDocument* doc = aLoadingContext->OwnerDoc();
> +      if (doc) {

Christoph is right. This nullcheck isn't needed. (Per Gecko naming convetions, that's why the function is called OwnerDoc() rather than GetOwnerDoc()).

::: netwerk/ipc/NeckoParent.cpp
@@ +109,5 @@
> +                                         uint32_t aSecurityFlags)
> +{
> +  nsresult rv;
> +  uint32_t appIdContext;
> +  rv =aLoadContext->GetAppId(&appIdContext);

Space after '='

@@ +304,5 @@
> +    if (NS_FAILED(rv)) {
> +      NS_WARNING("NeckoParent::AllocPHttpChannelParent: No loading principal");
> +      return nullptr;
> +    }
> +    rv = CheckLoadContextAndLoadInfo(loadContext, loadPrincipal,

I'm worried about doing this just for e10s builds. They see much less testing so it's very possible that this'll break something in FirefoxOS and desktop e10s without us noticing.

I'd really like to check that the loadContext and the loadInfo match also in normal builds. At the very least assert if there's a mismatch.

At least for http requests.

Would that be doable?
Assignee

Comment 10

4 years ago
It is possible to check it in not e10s build (in nsHttpChannel). I will update the patch.
Assignee

Comment 11

4 years ago
Posted patch bug_1125916_part1_v1.patch (obsolete) — Splinter Review
Attachment #8558509 - Attachment is obsolete: true
Attachment #8558509 - Flags: feedback?(jonas)
Attachment #8558509 - Flags: feedback?(jduell.mcbugs)
Assignee

Comment 12

4 years ago
Posted patch bug_1125916_part1_v1.patch (obsolete) — Splinter Review
Attachment #8574897 - Attachment is obsolete: true
Attachment #8575244 - Flags: review?(jonas)
Attachment #8575244 - Flags: review?(jduell.mcbugs)
Assignee

Comment 13

4 years ago
I am still working on a non e10s version, because some tests are failing. I am looking into it now.
Assignee

Comment 14

4 years ago
Posted patch bug_1125916_part2_v1.patch (obsolete) — Splinter Review
This is patch that is checking if appId, private browsing anf isInBrowserElement match just before AsyncOpen is called. This part of the code will be called for e10s and for not e10s build.
So for not e10s build it is breaking some tests. I am posting the patch if someone else wants to take a look as well, maybe Christoph?
Assignee

Comment 15

4 years ago
This is a try run with a similar patch as the one that I have submitted here. Both patches will break some tests in not e10s version.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=33370ca25b97
Assignee

Updated

4 years ago
Summary: e10s: new security API: replace SerializedLoadContext with Principals → e10s: new security API: replace SerializedLoadContext with LoadInfo
Comment on attachment 8575244 [details] [diff] [review]
bug_1125916_part1_v1.patch

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

The code looks good, but two questions:

Does this run these checks even in non-e10s mode? I.e. even for channels created in the parent?

Also, I would have thought that just security checks that should be guarded by a UsingNeckoIPCSecurity() check? And that these assertions are always a good idea to run.
Assignee

Comment 17

4 years ago
(In reply to Jonas Sicking (:sicking) from comment #16)
> Comment on attachment 8575244 [details] [diff] [review]
> bug_1125916_part1_v1.patch
> 
> Review of attachment 8575244 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The code looks good, but two questions:
> 
> Does this run these checks even in non-e10s mode? I.e. even for channels
> created in the parent?

This will run only in the e10s mode.
The second patch does the checks in nsHttpChannel and it would run in non-e10s mode as well, but it is failing. I intended to needinfo Christoph to take a look and help solving issues.


> 
> Also, I would have thought that just security checks that should be guarded
> by a UsingNeckoIPCSecurity() check? And that these assertions are always a
> good idea to run.

I wasn't sure if it would work in the case UsingNeckoIPCSecurity() is false, but if you thing it is good idea i will change that.
Assignee

Comment 18

4 years ago
Hi Christoph, 

The second patch,  bug_1125916_part2_v1.patch, is checking if information in loadInfo and loadContext match in e10s and non-e10s mode but they do not. If you have time to take a look. (see comment #14 and #15) Checks in the first patch,  bug_1125916_part1_v1.patch, seems to work but they are performed only in e10s mode
Flags: needinfo?(mozilla)
(In reply to Dragana Damjanovic [:dragana] from comment #18)
> Hi Christoph, 
> 
> The second patch,  bug_1125916_part2_v1.patch, is checking if information in
> loadInfo and loadContext match in e10s and non-e10s mode but they do not. If
> you have time to take a look. (see comment #14 and #15) Checks in the first
> patch,  bug_1125916_part1_v1.patch, seems to work but they are performed
> only in e10s mode

I applied your patches and ran a bunch of mochitests; the only error I encounter is in combination with the safebrowsing channel [1], where we use the SystemPrincipal as the loadingPrincipal but provide a custom LoadContext setting the SAFEBROWSING_APP_ID [2].

I think we have two options:
1) We provide a custom principal (using the SAFEBROWSING_APP_ID) when creating the safebrowsing channel instead of using systemPrincipal, or
2) we special case the safebrowsing case within CheckLoadInfo()

I rather lean towards (2), because the safebrowsing channel should bypass security checks once we move security checks into asyncOpen(). If we update the loadingPrincipal for the safebrowsing channel, we might run the risk of potentially blocking the safebrowsing channel. Jason, which option do you prefer?

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#136
[2] http://mxr.mozilla.org/mozilla-central/source/caps/nsIScriptSecurityManager.idl#226
Flags: needinfo?(mozilla) → needinfo?(jduell.mcbugs)
Comment on attachment 8575945 [details] [diff] [review]
bug_1125916_part2_v1.patch

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

Are we planning to land that code, or is it just for testing? If we land this then we should ifdef and only run it in debug builds. Either way, it would be great to put the testcode in more ::AsyncOpen() implementations and at least push to try to verify that we are operating on the same data.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4784,5 @@
> +{
> +  nsCOMPtr<nsILoadContext> loadContext;
> +  NS_QueryNotificationCallbacks(this, loadContext);
> +
> +  if (mLoadInfo && loadContext) {

You could use early returns to clean up that funciton in case mLoadInfo or loadContext are not available.

@@ +4832,5 @@
> +
> +    MOZ_ASSERT(loadingPrincipalIsInBE == loadContextIsInBE,
> +               "The isInBrowserElement the load context and in the loadinfo "
> +               "are not the same!");
> +  }

missing an return NS_OK here.
Attachment #8575945 - Flags: feedback+
Assignee

Comment 21

4 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #19)
> (In reply to Dragana Damjanovic [:dragana] from comment #18)
> > Hi Christoph, 
> > 
> > The second patch,  bug_1125916_part2_v1.patch, is checking if information in
> > loadInfo and loadContext match in e10s and non-e10s mode but they do not. If
> > you have time to take a look. (see comment #14 and #15) Checks in the first
> > patch,  bug_1125916_part1_v1.patch, seems to work but they are performed
> > only in e10s mode
> 
> I applied your patches and ran a bunch of mochitests; the only error I
> encounter is in combination with the safebrowsing channel [1], where we use
> the SystemPrincipal as the loadingPrincipal but provide a custom LoadContext
> setting the SAFEBROWSING_APP_ID [2].
> 
> I think we have two options:
> 1) We provide a custom principal (using the SAFEBROWSING_APP_ID) when
> creating the safebrowsing channel instead of using systemPrincipal, or
> 2) we special case the safebrowsing case within CheckLoadInfo()
> 
> I rather lean towards (2), because the safebrowsing channel should bypass
> security checks once we move security checks into asyncOpen(). If we update
> the loadingPrincipal for the safebrowsing channel, we might run the risk of
> potentially blocking the safebrowsing channel. Jason, which option do you
> prefer?


The point of this exercise is to check if loadinfo and loadcontext match, because sometime in (near) future we want to switch from loadcontext to loadinfo and eliminate loadcontext all together (maybe i am wrong). So for this error actually loadcontext is correct and loadinfo isn't. For me the option 1 makes more sense because we should go in that direction anyway, or is it too early for this change and you need more time.  Maybe i am wrong ...
Assignee

Comment 22

4 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #20)
> Comment on attachment 8575945 [details] [diff] [review]
> bug_1125916_part2_v1.patch
> 
> Review of attachment 8575945 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Are we planning to land that code, or is it just for testing? If we land
> this then we should ifdef and only run it in debug builds. Either way, it
> would be great to put the testcode in more ::AsyncOpen() implementations and
> at least push to try to verify that we are operating on the same data.
> 


We do plan to land this and run it for some time for testing, You are right this should be only in debug build or leave it in only till beta and than take it out. 
This is most use asyncOpen I will check where else i can put it in (ftp is not really used that much, maybe udpsocket, tcpsocket but they are a bit different)
Comment on attachment 8575244 [details] [diff] [review]
bug_1125916_part1_v1.patch

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

Comment 17 sounds good. But yes, I think you should remove the UsingNeckoIPCSecurity check.
Attachment #8575244 - Flags: review?(jonas) → review+
(In reply to Dragana Damjanovic [:dragana] from comment #21)
> The point of this exercise is to check if loadinfo and loadcontext match,
> because sometime in (near) future we want to switch from loadcontext to
> loadinfo and eliminate loadcontext all together (maybe i am wrong).

Yes, this is a goal, at least to me.

> So for this error actually loadcontext is correct and loadinfo isn't.

I would think so too.
Any news here?
Reporter

Updated

4 years ago
Attachment #8575244 - Flags: review?(jduell.mcbugs) → review+
Reporter

Comment 26

4 years ago
Comment on attachment 8575945 [details] [diff] [review]
bug_1125916_part2_v1.patch

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4784,5 @@
> +{
> +  nsCOMPtr<nsILoadContext> loadContext;
> +  NS_QueryNotificationCallbacks(this, loadContext);
> +
> +  if (mLoadInfo && loadContext) {

So normally it would be a serious error for mLoadInfo or loadContext not to be set? IIUC (which I may not) that should only happen during xpcshell tests.  In which case maybe we're better off making it an error for one or both of these to be null unless UsingNeckoSecurity().  But Jonas seemed to think it was fine to remove the UsingNeckoSec line, so it's quite possible I'm not understanding something.

@@ +4831,5 @@
> +    }
> +
> +    MOZ_ASSERT(loadingPrincipalIsInBE == loadContextIsInBE,
> +               "The isInBrowserElement the load context and in the loadinfo "
> +               "are not the same!");

So these checks are the same as the one in the other patch for NeckoParent, right?  If so we should probably centralize them into one function.
Attachment #8575945 - Flags: review+
Reporter

Updated

4 years ago
Flags: needinfo?(jduell.mcbugs)
Assignee

Updated

4 years ago
Summary: e10s: new security API: replace SerializedLoadContext with LoadInfo → e10s: new security API: replace SerializedLoadContext with LoadInfo - Check if information match
Assignee

Updated

4 years ago
Summary: e10s: new security API: replace SerializedLoadContext with LoadInfo - Check if information match → e10s: new security API: replace SerializedLoadContext with LoadInfo - Check if they match
Assignee

Updated

4 years ago
Depends on: 1175926
Dragana, any eta on this bug?
Flags: needinfo?(dd.mozilla)
FWIW, I think we need to add an optional OriginAttributes attribute to nsILoadInfo which enables a callsite to make a request with a different cookiejar. However we'll also need to remember to do security checks in the parent process to make sure this doesn't enable child processes from using cookies that they shouldn't be allowed to use.
blocking-b2g: --- → 2.5+
In what way does this affect the addons work? I.e. why does this block bug 1192026?
(In reply to Jonas Sicking (:sicking) from comment #41)
> In what way does this affect the addons work? I.e. why does this block bug
> 1192026?

We currently need to set network.disable.ipc.security to false to install *and run* add-ons because of necko security checks that fail for chrome callers (they look for a TabChild). My understanding is that we need this bug fixed to solve that.
blocking-b2g: 2.5+ → ---
feature-b2g: --- → 2.5+
Assignee

Comment 43

4 years ago
Because I do not know some part of the code :ckerschb is helping me resolve some of bugs this bug is depending on.
Flags: needinfo?(dd.mozilla)
No longer blocks: addons25
Assignee

Comment 44

4 years ago
Posted patch bug_1125916_part1_v1.patch (obsolete) — Splinter Review
rebased
Attachment #8575244 - Attachment is obsolete: true
Attachment #8658560 - Flags: review+
Assignee

Comment 45

4 years ago
Posted patch bug_1125916_part2_v1.patch (obsolete) — Splinter Review
rebased.
Attachment #8575945 - Attachment is obsolete: true
Attachment #8658562 - Flags: review+
Dragana asked me to help debugging test_appIsolation.html [1] and why the AppId of the loadingPrincipal differs from the AppId of the loadContext. When applying the attached patch I get the following error (including a small stacktrace).

> LoadInfoVSLoadContext {
>  uri: http://mochi.test:8888/tests/dom/tests/mochitest/localstorage/frameAppIsolation.html?read-no
>  loadingPrincipal: http://mochi.test:8888/tests/dom/tests/mochitest/localstorage/test_appIsolation.html
>  loadInfo->AppId: 0
>  loadContext->AppId: 1
>  loadInfo->isInBrowserElement: false
>  loadingPrincipal->isInBrowserElement: false
>  isApp: yes
> }
> Assertion failure: loadInfoAppId == loadContextAppId (uaaahh - appIDs different), at /Users/ckerschb/Documents/moz/mc/netwerk/protocol/http/nsHttpChannel.cpp:6988
> #01: mozilla::net::LoadInfoVSLoadContext(nsIInterfaceRequestor*, nsILoadInfo*, nsIURI*) (nsHttpChannel.cpp:6987, in XUL)
> #02: mozilla::net::nsHttpChannel::SetNotificationCallbacks(nsIInterfaceRequestor*) (nsHttpChannel.cpp:7001, in XUL)
> #03: non-virtual thunk to mozilla::net::nsHttpChannel::SetNotificationCallbacks(nsIInterfaceRequestor*) (nsHttpChannel.cpp:7006, in XUL)
> #04: NS_NewChannelInternal(nsIChannel**, nsIURI*, nsINode*, nsIPrincipal*, nsIPrincipal*, unsigned int, unsigned int, nsILoadGroup*, nsIInterfaceRequestor*, unsigned int, nsIIOService*) (nsNetUtil.inl:175, in XUL)
> #05: nsDocShell::DoURILoad(nsIURI*, nsIURI*, bool, unsigned int, nsISupports*, char const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, bool, nsIDocShell**, nsIRequest**, bool, bool, bool, nsAString_internal const&, nsIURI*, unsigned int) (nsDocShell.cpp:10419, in XUL)
> #06: nsDocShell::InternalLoad(nsIURI*, nsIURI*, unsigned int, nsISupports*, unsigned int, char16_t const*, char const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, bool, nsAString_internal const&, nsIDocShell*, nsIURI*, nsIDocShell**, nsIRequest**) (nsDocShell.cpp:10216, in XUL)
> #07: nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, bool) (nsDocShell.cpp:1549, in XUL)
> #08: non-virtual thunk to nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, bool) (nsDocShell.cpp:1567, in XUL)
> #09: nsFrameLoader::ReallyStartLoadingInternal() (nsFrameLoader.cpp:442, in XUL)
> #10: nsFrameLoader::ReallyStartLoading() (nsFrameLoader.cpp:326, in XUL)

It seems that AppIds for apps are custom modified within docShell. The AppId in question gets set here (including a small stacktrace):
> #01: nsDocShell::SetIsApp(unsigned int) (nsDocShell.cpp:13598, in XUL)
> #02: non-virtual thunk to nsDocShell::SetIsApp(unsigned int) (nsDocShell.cpp:13608, in XUL)
> #03: nsFrameLoader::MaybeCreateDocShell() (nsFrameLoader.cpp:1875, in XUL)
> #04: nsFrameLoader::CheckForRecursiveLoad(nsIURI*) (nsFrameLoader.cpp:1950, in XUL)
> #05: nsFrameLoader::CheckURILoad(nsIURI*) (nsFrameLoader.cpp:487, in XUL)
> #06: nsFrameLoader::LoadURI(nsIURI*) (nsFrameLoader.cpp:267, in XUL)
> #07: nsFrameLoader::LoadFrame() (nsFrameLoader.cpp:233, in XUL)
> #08: nsGenericHTMLFrameElement::LoadSrc() (nsGenericHTMLFrameElement.cpp:218, in XUL)
> #09: nsGenericHTMLFrameElement::BindToTree(nsIDocument*, nsIContent*, nsIContent*, bool) (nsGenericHTMLFrameElement.cpp:247, in XUL)
> #10: nsINode::doInsertChildAt(nsIContent*, unsigned int, bool, nsAttrAndChildArray&) (nsINode.cpp:1587, in XUL)

Jonas, am I correct with the assumption that once we add 'GetAppId()' [*if* that happens before OriginAttributes()] to the loadInfo, we not only have to return a special AppId for SafeBrowsing but also have to take into account whether we are loading an app or not?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/localstorage/test_appIsolation.html?force=1
Flags: needinfo?(jonas)
Assignee

Comment 47

4 years ago
A version with OriginAttributes added to Loadinfo.
Assignee

Comment 48

4 years ago
A version with OriginAttributes added to Loadinfo.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #46)
> Jonas, am I correct with the assumption that once we add 'GetAppId()' [*if*
> that happens before OriginAttributes()] to the loadInfo, we not only have to
> return a special AppId for SafeBrowsing but also have to take into account
> whether we are loading an app or not?

Once we add GetAppId/OriginAttributes() on LoadInfo, it should be possible for someone creating a channel to set what appid/originAttributes returns.

So the safebrowsing code can then set the appid to be the safebrowsing appid.

The loadinfo code shouldn't have any special code for neither safebrowsing nor apps. It'll just get the appid/originattributes from the loadingPrincipal and then allow it to be overridden later.
Flags: needinfo?(jonas)
Assignee

Comment 50

4 years ago
It is the same as original patch, it is only change to use OriginAttributes.
Attachment #8658560 - Attachment is obsolete: true
Attachment #8664249 - Attachment is obsolete: true
Attachment #8666710 - Flags: review?(jduell.mcbugs)
Assignee

Comment 51

4 years ago
This one is also using OriginAttributes. the rest is the same.
Attachment #8658562 - Attachment is obsolete: true
Attachment #8664250 - Attachment is obsolete: true
Attachment #8666711 - Flags: review?(jduell.mcbugs)
Assignee

Comment 52

4 years ago
Posted patch bug_1175926_v1.patch (obsolete) — Splinter Review
This patch is closely related to this bug, so pushing this one without other 2 will not work.

Christoph can you take a look at this. I am not familiar with most of this code, so for some parts I am not sure.
Attachment #8666715 - Flags: feedback?(mozilla)
Reporter

Comment 53

4 years ago
Comment on attachment 8666710 [details] [diff] [review]
bug_1125916_part1_withOriginAttributes_v1.patch

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

::: netwerk/base/nsILoadInfo.idl
@@ +136,5 @@
> +   *
> +   * If the flag is true, even if a document context is present,
> +   * GetUsePrivateBrowsing will always return true.
> +   */
> +  const unsigned long SEC_FORCE_PRIVATE_BROWSING = (1<<10);

So our whole "force private browsing" architecture was kludged together by ehsan in a patch that even he admits was an awful hack (see bug 741059 comment 12) in order to allow call sites that lacked an nsILoadContext (notably the favicon loader) to still set PB for a channel.  

It's a horrible architecture. If/when we've actually started making sure that all network loads have a nsILoadInfo (I'm not sure if that's true?) I'd really, really, love to have all PB decisions made via the LoadInfo, and we could get rid of the entire nsIPrivateBrowsingChannel infrastructure and just use nsILoadInfo.  That's probably a new bug for some happy time in the future.

But in the meantime, it's confusing to call this new flag SEC_FORCE_PRIVATE_BROWSING.  It's weird to see your nsDocShell patch setting this flag when it determines private browsing is being used--the whole point of "forcing" PB was for contexts where we don't have a docshell.  

I'm also confused by the comment that if set, GetUsePrivateBrowsing will return true "even if a document context is present".  The function isn't checking document context whether or not this flag is set.  Do you mean NS_UsePrivateBrowsing? (but that function is still checking nsILoadContext, not loadinfo).  I'm not actually clear on what setting the PB flag in the loadInfo is actually doing at the moment if this patch lands (seems like we're still using loadContext and nsIPrivateBrowsingChannel for all decisions on whether to use PB).

Assuming the long-term goal here is to have all network channels created with a loadInfo (and to purge the awfulness that is "force PB"), it's probably better to just call this SEC_USE_PRIVATE_BROWSING?

::: netwerk/ipc/NeckoParent.cpp
@@ +112,5 @@
> +nsresult
> +NeckoParent::CheckLoadContextAndLoadInfo(nsILoadContext *aLoadContext,
> +                                         uint32_t aAppId,
> +                                         bool aInBrowser,
> +                                         uint32_t aSecurityFlags)

any reason why we shouldn't just pass in a LoadInfoArgs& instead of making each call site extract the 3 subvalues?

@@ +142,5 @@
> +
> +  bool usePrivateBrowsingContext;
> +  rv = aLoadContext->GetUsePrivateBrowsing(&usePrivateBrowsingContext);
> +  if (NS_FAILED(rv)) {
> +    NS_WARNING("NeckoParent::CheckLoadContextAndLoadInfo: EATAL error - "

FATAL?

@@ +282,5 @@
> +      mozilla::ipc::PrincipalInfoToPrincipal(
> +        loadInfoArgs.requestingPrincipalInfo(), &rv);
> +    if (NS_FAILED(rv)) {
> +      NS_WARNING("NeckoParent::AllocPHttpChannelParent: No loading principal");
> +      return nullptr;

better to use printf_stderr (as rest of function does) than NS_WARNING for an error that will kill the child?  At least that's what we've been using.  (IIRC printf_stderr was much more visible to B2G developers: maybe that's changed).

That's also true for all the uses of NS_WARNING in CheckLoadContextandLoadInfo().

@@ +344,5 @@
> +        loadInfoArgs.requestingPrincipalInfo(),
> +        &rv);
> +    if (NS_FAILED(rv)) {
> +      NS_WARNING("NeckoParent::AllocPTFPChannelParent: No loading principal");
> +      return nullptr;

printf_stderr
Attachment #8666710 - Flags: review?(jduell.mcbugs) → review+
Reporter

Comment 54

4 years ago
Comment on attachment 8666711 [details] [diff] [review]
bug_1125916_part2_withOriginAttributes_v1.patch

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5065,5 @@
>      return rv;
>  }
>  
> +nsresult
> +nsHttpChannel::CheckLoadInfo()

A little bit of a pity that we can't share any of the logic from NeckoParent::CheckLoadContextAndLoadInfo().  But no big deal.

@@ +5104,5 @@
> +         loadContextAppId, loadContextPB, loadContextIsInBE,
> +         this));
> +
> +    MOZ_ASSERT(mLoadInfo->GetOriginAttributes().mAppId == loadContextAppId,
> +               "The appId the load context and in the loadinfo are not the "

The appId *in* the load context...

@@ +5107,5 @@
> +    MOZ_ASSERT(mLoadInfo->GetOriginAttributes().mAppId == loadContextAppId,
> +               "The appId the load context and in the loadinfo are not the "
> +               "same!");
> +    MOZ_ASSERT(loadInfoUsePB == loadContextPB,
> +               "The usePrivateBrowsing the load context and in the loadinfo "

*in* the load context

@@ +5110,5 @@
> +    MOZ_ASSERT(loadInfoUsePB == loadContextPB,
> +               "The usePrivateBrowsing the load context and in the loadinfo "
> +               "are not the same!");
> +    MOZ_ASSERT(mLoadInfo->GetOriginAttributes().mInBrowser == loadContextIsInBE,
> +               "The isInBrowserElement the load context and in the loadinfo "

ditto
Attachment #8666711 - Flags: review?(jduell.mcbugs) → review+
Comment on attachment 8666710 [details] [diff] [review]
bug_1125916_part1_withOriginAttributes_v1.patch

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

Thanks Dragana, just some nits.

::: netwerk/base/LoadInfo.cpp
@@ +52,5 @@
>    }
>  
> +
> +  if (!(mSecurityFlags & nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING)) {
> +    if (aLoadingContext) {

A little further down in the method we are already do if (aLoadContext) - can you please move your check within that if-clause? This way we can avoid the additonal if check.

@@ +263,5 @@
>  NS_IMETHODIMP
> +LoadInfo::GetUsePrivateBrowsing(bool* aUsePrivateBrowsing)
> +{
> +  *aUsePrivateBrowsing = (mSecurityFlags &
> +                          nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING);

Nit: please write in the same style as the other Get*Flag* methods:

*aUsePrivateBrowsing =
  (mSecurityFlags & nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING);

::: netwerk/base/nsILoadInfo.idl
@@ +134,5 @@
> +   * Force private browsing. Setting this flag the private browsing can be
> +   * enforce even when a loading is not happening in the context of a document.
> +   *
> +   * If the flag is true, even if a document context is present,
> +   * GetUsePrivateBrowsing will always return true.

Nit:

Force private browsing. Setting this flag allows to always force private browsing even if a document context is present.

::: netwerk/ipc/NeckoParent.cpp
@@ +347,5 @@
> +      NS_WARNING("NeckoParent::AllocPTFPChannelParent: No loading principal");
> +      return nullptr;
> +    }
> +
> +    rv = CheckLoadContextAndLoadInfo(loadContext, 

Nit: trailing whitespace.

::: netwerk/ipc/NeckoParent.h
@@ +37,5 @@
>  
> +  nsresult CheckLoadContextAndLoadInfo(nsILoadContext *aLoadContext,
> +                                       uint32_t aAppId,
> +                                       bool aInBrowser,
> +                                       uint32_t aSecurityFlags);

Nit: This function is only called from within NeckoParent.cpp - there is no need of making this part of the class, right?
Attachment #8666710 - Flags: feedback+
Comment on attachment 8666711 [details] [diff] [review]
bug_1125916_part2_withOriginAttributes_v1.patch

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4993,5 @@
>                 "security flags in loadInfo but asyncOpen2() not called");
>  
>      LOG(("nsHttpChannel::AsyncOpen [this=%p]\n", this));
>  
> +    CheckLoadInfo();

I am just curious: Here we call checkLoadInfo() in nsHttpChannel, but in the e10s patch we are calling checkLoadInfo() within NeckoParent. Couldn't we also call checkLoadInfo within HttpChannelParent::DoAsyncOpen()? If so we could use the same *CheckLoadinfo* function for e10s and non e10s mode, or am I missing something important here?

@@ +5070,5 @@
> +{
> +  nsCOMPtr<nsILoadContext> loadContext;
> +  NS_QueryNotificationCallbacks(this, loadContext);
> +
> +  if (mLoadInfo && loadContext) {

Nit: please use early returns where you can.
Dragana, please see my question in comment 56.
Flags: needinfo?(dd.mozilla)
Comment on attachment 8666715 [details] [diff] [review]
bug_1175926_v1.patch

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

Hey Dragana, it looks good from a first glance. I am curious, how did you find all these places? Did you push to TRY and fixed what breaks? I am mentioning that in my review comment underneath again, but for now we have to make sure that we have null checks before accessing the loadInfo. There are only a few places which are non tests where we definitely need separate reviews by peers. I am pretty sure Jonas can help on a lot of them once we have the nullchecks on the loadInfo. Thanks!

::: docshell/base/nsDocShell.cpp
@@ +10438,5 @@
> +                               securityFlags,
> +                               aContentPolicyType,
> +                               nullptr,   // loadGroup
> +                               static_cast<nsIInterfaceRequestor*>(this),
> +                               loadFlags);

What changed here within NS_NewChannelInternal()?

@@ +10459,5 @@
> +
> +    nsCOMPtr<nsILoadInfo> loadInfo;
> +    rv = channel->GetLoadInfo(getter_AddRefs(loadInfo));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    if (mOwnOrContainingAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID) {

Please query the loadinfo within the if statement. Btw, since recently you can simplify the statement to
nsCOMPtr<nsILoadInfo> loadInfo = channel->GetLoadInfo();

Please be aware that not all channels have a loadInfo attached, e.g. some addon channels might not provide a loadInfo, hence we *always* need to perform a nullCheck.

::: dom/apps/Webapps.jsm
@@ +2251,5 @@
>                    .createInstance(Ci.nsIXMLHttpRequest);
>        xhr.open("GET", aData.manifestURL, true);
>        xhr.channel.loadFlags |= Ci.nsIRequest.INHIBIT_CACHING;
> +      xhr.channel.loadInfo.originAttributes = { appId: app.installerAppId,
> +                                                inBrowser: app.installerIsBrowser};

here and elsewhere the loadInfo might be null.
Attachment #8666715 - Flags: feedback?(mozilla) → feedback+
Assignee

Comment 59

4 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #56)
> Comment on attachment 8666711 [details] [diff] [review]
> bug_1125916_part2_withOriginAttributes_v1.patch
> 
> Review of attachment 8666711 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +4993,5 @@
> >                 "security flags in loadInfo but asyncOpen2() not called");
> >  
> >      LOG(("nsHttpChannel::AsyncOpen [this=%p]\n", this));
> >  
> > +    CheckLoadInfo();
> 
> I am just curious: Here we call checkLoadInfo() in nsHttpChannel, but in the
> e10s patch we are calling checkLoadInfo() within NeckoParent. Couldn't we
> also call checkLoadInfo within HttpChannelParent::DoAsyncOpen()? If so we
> could use the same *CheckLoadinfo* function for e10s and non e10s mode, or
> am I missing something important here?

The e10s version came first because it started with e10s, even the title of the bug says so :)
And the e10s version has not break so may tests as the not-e10s version, that is why i have kept it.


So we could remove check in NeckoParent  and even in HttpChannelParent because it is calling nsHttpChannel::AsyncOpen right the way in any case.
I will change this.

There is 1 or 2 tests that is still failing, but i think that it is not fault of this bug (I need to check this once more)
Flags: needinfo?(dd.mozilla)
Assignee

Comment 60

4 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #58)
> Comment on attachment 8666715 [details] [diff] [review]
> bug_1175926_v1.patch
> 
> Review of attachment 8666715 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hey Dragana, it looks good from a first glance. I am curious, how did you
> find all these places? Did you push to TRY and fixed what breaks? I am
> mentioning that in my review comment underneath again, but for now we have
> to make sure that we have null checks before accessing the loadInfo. There
> are only a few places which are non tests where we definitely need separate
> reviews by peers. I am pretty sure Jonas can help on a lot of them once we
> have the nullchecks on the loadInfo. Thanks!
> 
> ::: docshell/base/nsDocShell.cpp
> @@ +10438,5 @@
> > +                               securityFlags,
> > +                               aContentPolicyType,
> > +                               nullptr,   // loadGroup
> > +                               static_cast<nsIInterfaceRequestor*>(this),
> > +                               loadFlags);
> 
> What changed here within NS_NewChannelInternal()?
> 

nothing I just fixed alignment.

I will fix nsILoadInfo part.
Dragana, are you going to land this?
Flags: needinfo?(dd.mozilla)
Priority: -- → P3
Assignee

Comment 62

3 years ago
originAttributes was change, involving at the time I made this patches, so I have decided to wait a bit. I will land this, soon.
Flags: needinfo?(dd.mozilla)
Whiteboard: [necko-active]
Assignee

Comment 69

3 years ago
Attachment #8666710 - Attachment is obsolete: true
Attachment #8727398 - Flags: review?(jonas)
Attachment #8727398 - Flags: review?(jduell.mcbugs)
Assignee

Comment 70

3 years ago
Attachment #8666711 - Attachment is obsolete: true
Attachment #8727400 - Flags: review?(jonas)
Attachment #8727400 - Flags: review?(jduell.mcbugs)
Assignee

Comment 71

3 years ago
This is a fix for bugs 1175695 1175700 1175704 1175926 that are caused by this loadInfo-loadContext check.
Attachment #8666715 - Attachment is obsolete: true
Attachment #8727401 - Flags: review?(jonas)
Attachment #8727401 - Flags: review?(jduell.mcbugs)
Comment on attachment 8727398 [details] [diff] [review]
bug_1125916_part1_withOriginAttributes_v1.patch

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

Looks good to me.

::: netwerk/base/LoadInfo.cpp
@@ +99,5 @@
>  
> +  if (!(mSecurityFlags & nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING)) {
> +    if (aLoadingContext) {
> +      nsCOMPtr<nsILoadContext> loadContext =
> +        aLoadingContext->OwnerDoc()->GetLoadContext();

There's a number of cases where this won't work because we don't have a aLoadingContext. For example for any requests from workers.

But I take it that the plan is that in those cases we'll want the caller to explicitly pass in the EC_FORCE_PRIVATE_BROWSING as appropriate?
Attachment #8727398 - Flags: review?(jonas) → review+
Comment on attachment 8727400 [details] [diff] [review]
bug_1125916_part2_withOriginAttributes_v1.patch

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

r=me if we can refactor this somewhere so that the code can be shared.

::: netwerk/base/nsBaseChannel.cpp
@@ +945,5 @@
> +    rv = mLoadInfo->GetUsePrivateBrowsing(&loadInfoUsePB);
> +    if (NS_FAILED(rv)) {
> +      return NS_ERROR_UNEXPECTED;
> +    }
> +    bool loadContextPB = false;

"loadContextUsePB" to be consistent with "loadInfoUsePB" above?

@@ +952,5 @@
> +      return NS_ERROR_UNEXPECTED;
> +    }
> +
> +    bool loadContextIsInBE = false;
> +    rv = loadContext->GetIsInIsolatedMozBrowserElement(&loadContextIsInBE);

Nit: Move this up to below the loadContextAppId section since the two belong together (and both are part of OriginAttributes)

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +3273,5 @@
>    return NS_OK;
>  }
>  
> +nsresult
> +HttpBaseChannel::CheckLoadInfo()

Hmm.. can we move this into a helper function somewhere in necko instead? Seems unfortunate to duplicate.

I don't really have an opinion about where in necko though.
Attachment #8727400 - Flags: review?(jonas) → review+
Comment on attachment 8727400 [details] [diff] [review]
bug_1125916_part2_withOriginAttributes_v1.patch

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

::: netwerk/base/nsBaseChannel.cpp
@@ +927,5 @@
>    return listener->CheckListenerChain();
>  }
> +
> +nsresult
> +nsBaseChannel::CheckLoadInfo()

Oh, you should also check that the userContextId match between the OriginAttributes and the nsILoadContext.
Comment on attachment 8727401 [details] [diff] [review]
bug_1125916_part3_fix_tests_v1.patch

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

Looks good other than for the HTMLMediaElement.cpp and imgLoader.cpp changes.

::: docshell/base/nsDocShell.cpp
@@ +10642,5 @@
> +    nsCOMPtr<nsILoadInfo> loadInfo = channel->GetLoadInfo();
> +    if (loadInfo) {
> +        NeckoOriginAttributes neckoAttrs;
> +        neckoAttrs.InheritFromDocShellToNecko(GetOriginAttributes());
> +        loadInfo->SetOriginAttributes(neckoAttrs);

Oh, this is tricky.. but yeah, we definitely need to do this. Can you move this code to right after we create the loadinfo though:

http://hg.mozilla.org/mozilla-central/file/be593a64d7c6/docshell/base/nsDocShell.cpp#l10636

It's something we should definitely do before we create the channel.

Also maybe add some comment like:

"We have to do this in case our OriginAttributes are different from the OriginAttributes of the parent document. Or in case there isn't a parent document".

::: dom/html/HTMLMediaElement.cpp
@@ +1311,5 @@
> +    docShellPtr = nsDocShell::Cast(docShell);
> +    bool privateBrowsing;
> +    docShellPtr->GetUsePrivateBrowsing(&privateBrowsing);
> +    if (privateBrowsing) {
> +      securityFlags |= nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING;

Why is this needed? Won't the loadInfo ctor pick up the correct PB flag from the loadingNode (aka loadingContext)?

@@ +1335,5 @@
> +  nsCOMPtr<nsILoadInfo> loadInfo = channel->GetLoadInfo();
> +  if (loadInfo) {
> +    NeckoOriginAttributes originAttrs;
> +    NS_GetOriginAttributes(channel, originAttrs);
> +    loadInfo->SetOriginAttributes(originAttrs);

Same here? Why don't we pick up the correct OriginAttributes from the loadingNode?

::: image/imgLoader.cpp
@@ +853,5 @@
> +      DocShellOriginAttributes originAttrs;
> +      loadContext->GetOriginAttributes(originAttrs);
> +      NeckoOriginAttributes neckoOriginAttrs;
> +      neckoOriginAttrs.InheritFromDocShellToNecko(originAttrs);
> +      loadInfo->SetOriginAttributes(neckoOriginAttrs);

Same here, why is this needed?
Reporter

Comment 76

3 years ago
Comment on attachment 8727398 [details] [diff] [review]
bug_1125916_part1_withOriginAttributes_v1.patch

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

Should one of the patches in this bug update NS_UsePrivateBrowsing() in nsNetUtil.h to also check LoadInfo::GetUsePrivateBrowsing()? Or will that happen later, when we start ripping out nsILoadContext?

::: netwerk/base/nsILoadInfo.idl
@@ +28,5 @@
>  
>  /**
>   * An nsILoadOwner represents per-load information about who started the load.
>   */
> +[scriptable, builtinclass, uuid(3a8eed42-c9fc-4413-92b9-286805052089)]

We don't need to update UUIDs any more, and it's discouraged.
Attachment #8727398 - Flags: review?(jduell.mcbugs) → review+
Reporter

Comment 77

3 years ago
Comment on attachment 8727400 [details] [diff] [review]
bug_1125916_part2_withOriginAttributes_v1.patch

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

I'm also +r on the next rev of this if we can de-duplicate the CheckLoadInfo() logic.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +3273,5 @@
>    return NS_OK;
>  }
>  
> +nsresult
> +HttpBaseChannel::CheckLoadInfo()

I agree with Jonas that it would be nice to not duplicate this code in HttpBaseChannel::CheckLoadInfo() and nsBaseChannel::CheckLoadInfo().

Http and non-HTTP objects don't share any base class, but it looks like we could write a template function that will work on either?  (I haven't diff'd the two functions closely enough to see if there are any differences that would be a problem).
Attachment #8727400 - Flags: review?(jduell.mcbugs) → review+
Assignee

Comment 78

3 years ago
(In reply to Jonas Sicking (:sicking) from comment #72)
> Comment on attachment 8727398 [details] [diff] [review]
> bug_1125916_part1_withOriginAttributes_v1.patch
> 
> Review of attachment 8727398 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.
> 
> ::: netwerk/base/LoadInfo.cpp
> @@ +99,5 @@
> >  
> > +  if (!(mSecurityFlags & nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING)) {
> > +    if (aLoadingContext) {
> > +      nsCOMPtr<nsILoadContext> loadContext =
> > +        aLoadingContext->OwnerDoc()->GetLoadContext();
> 
> There's a number of cases where this won't work because we don't have a
> aLoadingContext. For example for any requests from workers.
> 
> But I take it that the plan is that in those cases we'll want the caller to
> explicitly pass in the EC_FORCE_PRIVATE_BROWSING as appropriate?

nsSecurityFlags are passed as a parameter to the constructor it would be appopriate to pass SEC_FORCE_PRIVATE_BROWSING flag as well.
Assignee

Comment 79

3 years ago
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #76)
> Comment on attachment 8727398 [details] [diff] [review]
> bug_1125916_part1_withOriginAttributes_v1.patch
> 
> Review of attachment 8727398 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Should one of the patches in this bug update NS_UsePrivateBrowsing() in
> nsNetUtil.h to also check LoadInfo::GetUsePrivateBrowsing()? Or will that
> happen later, when we start ripping out nsILoadContext?
> 

I will make a separate patch for this: part4, because I am afraid it is going to break some tests depending when it is call - proper values for LoadContext are set earlier than in LoadInfo in some cases.
Assignee

Comment 80

3 years ago
(In reply to Jonas Sicking (:sicking) from comment #75)
> Comment on attachment 8727401 [details] [diff] [review]
> bug_1125916_part3_fix_tests_v1.patch
> 
> Review of attachment 8727401 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> ::: dom/html/HTMLMediaElement.cpp
> @@ +1311,5 @@
> > +    docShellPtr = nsDocShell::Cast(docShell);
> > +    bool privateBrowsing;
> > +    docShellPtr->GetUsePrivateBrowsing(&privateBrowsing);
> > +    if (privateBrowsing) {
> > +      securityFlags |= nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING;
> 
> Why is this needed? Won't the loadInfo ctor pick up the correct PB flag from
> the loadingNode (aka loadingContext)?
> 
> @@ +1335,5 @@
> > +  nsCOMPtr<nsILoadInfo> loadInfo = channel->GetLoadInfo();
> > +  if (loadInfo) {
> > +    NeckoOriginAttributes originAttrs;
> > +    NS_GetOriginAttributes(channel, originAttrs);
> > +    loadInfo->SetOriginAttributes(originAttrs);
> 
> Same here? Why don't we pick up the correct OriginAttributes from the
> loadingNode?
> 
> ::: image/imgLoader.cpp
> @@ +853,5 @@
> > +      DocShellOriginAttributes originAttrs;
> > +      loadContext->GetOriginAttributes(originAttrs);
> > +      NeckoOriginAttributes neckoOriginAttrs;
> > +      neckoOriginAttrs.InheritFromDocShellToNecko(originAttrs);
> > +      loadInfo->SetOriginAttributes(neckoOriginAttrs);
> 
> Same here, why is this needed?

Some tests were failing (I forgot which one but I am going to find out).
I take another look.
Reporter

Comment 81

3 years ago
Comment on attachment 8727401 [details] [diff] [review]
bug_1125916_part3_fix_tests_v1.patch

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

Looks fine to me modulo Jonas's comments.  I assume there will be a new patch, I'm fine with just Jonas reviewing that one (i.e. you can count me as a +r if he's +r)
Attachment #8727401 - Flags: review?(jduell.mcbugs) → feedback+
Assignee

Comment 82

3 years ago
just rebased.
Attachment #8727398 - Attachment is obsolete: true
Attachment #8731110 - Flags: review+
Assignee

Comment 83

3 years ago
Made function: NS_CompareLoadInfoAndLoadContext(nsIChannel *aChannel)
no more 2 CheckLoadInfo functions.
Attachment #8727400 - Attachment is obsolete: true
Attachment #8731111 - Flags: review+
Assignee

Comment 84

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aee01bbb0811&selectedJob=18109877
This is a try run where code questioned in comment @75 has been commented out.

For example test dom/browser-element/mochitest/test_browserElement_inproc_ContextmenuEvents.html fails.

loadContext is docShell and OwnerDoc is nsHTMLDocument. docShell and nsHTMLDocument do not have a matching principal.
I do not know this code, I will ask ckerschb for help, or Jonas knows wht is wrong :)
Christoph can you take a look?
Flags: needinfo?(mozilla)
(In reply to Dragana Damjanovic [:dragana] from comment #84)
> Christoph can you take a look?

Hey Dragana, so far I was able to pinpoint the problem within the imgLoader. I applied the attached debug patch and ran test [1]. Right after creating the channel the loadcontext and the loadinfo values for InIsolatedMozBrowser don't match and the assertion within the debug patch triggers.

Reason being is that requestingNode is null and hence we create the channel using the systemPrincipal. I then applied the patches from Bug 1256999 where we try to pass the right context to image channels which then causes the assertion within the debug patch *not* to be triggered. The test still fails though because there is a similar problem within HTMLMediaElement (see comment 75) which also gets triggered within that test.

My suggestion is that we wait till Bug 1256999 lands before continuing. I can then have another look at what's going wrong within HTMLMediaElement.

[1] dom/browser-element/mochitest/test_browserElement_inproc_ContextmenuEvents.html
Flags: needinfo?(mozilla)
Assignee

Comment 86

3 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #85)
> Created attachment 8733050 [details] [diff] [review]
> imgloader_debug.patch
> 
> (In reply to Dragana Damjanovic [:dragana] from comment #84)
> > Christoph can you take a look?
> 
> Hey Dragana, so far I was able to pinpoint the problem within the imgLoader.
> I applied the attached debug patch and ran test [1]. Right after creating
> the channel the loadcontext and the loadinfo values for InIsolatedMozBrowser
> don't match and the assertion within the debug patch triggers.
> 
> Reason being is that requestingNode is null and hence we create the channel
> using the systemPrincipal. I then applied the patches from Bug 1256999 where
> we try to pass the right context to image channels which then causes the
> assertion within the debug patch *not* to be triggered. The test still fails
> though because there is a similar problem within HTMLMediaElement (see
> comment 75) which also gets triggered within that test.
> 
> My suggestion is that we wait till Bug 1256999 lands before continuing. I
> can then have another look at what's going wrong within HTMLMediaElement.
> 
> [1]
> dom/browser-element/mochitest/test_browserElement_inproc_ContextmenuEvents.
> html

Thanks a lot. I will wait for bug 1256999.
Assignee

Updated

3 years ago
Depends on: 1256999
(In reply to Dragana Damjanovic [:dragana] from comment #86)
> Thanks a lot. I will wait for bug 1256999.

In the meantime I will try to have a look what's wrong with HTMLMediaElement. Maybe we can get that problem resolved soonish as well. I would really like to see the patches in this bug landed. Just making sure, besides the imgLoader and the HTMLMediaElement there shouldn't be any other problems left for this bug, right?
Assignee

Comment 88

3 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #87)
> (In reply to Dragana Damjanovic [:dragana] from comment #86)
> > Thanks a lot. I will wait for bug 1256999.
> 
> In the meantime I will try to have a look what's wrong with
> HTMLMediaElement. Maybe we can get that problem resolved soonish as well. I
> would really like to see the patches in this bug landed. Just making sure,
> besides the imgLoader and the HTMLMediaElement there shouldn't be any other
> problems left for this bug, right?

There should not be an more problems. In the try run from comment #68 everything was fine (of course with the 2 hacks - imgLoad and HTMLMediaElement). 

I will run it again, just to be sure nothing has changed in the last week :)
Looking at the test failures from comment 84, I can see two failures:

test_focus_blur_manage_events.html

This appears to be a CSS stylesheet which is triggering an image load, and for that image load the information is not matching up.

This might very well be fixed by bug 1256999, but I would also not be surprised if it is not. We can certainly wait and see, but probably better to land the imgloader changes for now, and then try to remove it separately.

It might even be useful to have these changes landed while developing the patches in bug 1256999. That way we can try to revert the changes there, and if the assertion triggers, the patches in bug 1256999 likely need more work.


test_browserElement_inproc_ContextmenuEvents.html

This appears to be a problem in HTMLMediaElement. This is quite surprising since this is a request originating from a DOM node, which should make it quite straight-forward to get the information both the old way and the new way.

We should definitely look into this one. Why does the information through the OwnerDoc() not match the information in the nsILoadContext? Which one is "correct"?

It's also somewhat strange if there's something wrong in HTMLMediaElement, that it only triggers in this one test.

Given that this part *should* work already, I think it's worth looking at this a bit before landing this bug.
Assignee

Comment 90

3 years ago
rebased
Attachment #8731110 - Attachment is obsolete: true
Attachment #8740834 - Flags: review+
Assignee

Comment 91

3 years ago
imgLoad.cpp change has been removed.
HTMLMediaElement.cpp is still the same work around. The real fix will be in a different bug.
Attachment #8727401 - Attachment is obsolete: true
Attachment #8727401 - Flags: review?(jonas)
Attachment #8740838 - Flags: review?(ckerschb)
Assignee

Comment 93

3 years ago
(In reply to Dragana Damjanovic [:dragana] from comment #92)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e31c4b36c97

I should have tried it locally, before pushing to try.

So bug 1256999 has not fixed the loadInfo-loadContext mismatch.

Assertion failure: originAttrsLoadInfo.mInIsolatedMozBrowser == loadContextIsInBE (The value of InIsolatedMozBrowser in the loadContext and in the loadInfo are not the same!), at /home/mozilla/dragana_work/necko/mozilla-central/netwerk/base/nsNetUtil.cpp:2414
Assignee

Comment 94

3 years ago
This patch contains work around for imgload and HtmlMediaElement. The proper fix will be is separate bugs.
Attachment #8740838 - Attachment is obsolete: true
Attachment #8740838 - Flags: review?(ckerschb)
Attachment #8740857 - Flags: review?(ckerschb)
Comment on attachment 8740857 [details] [diff] [review]
bug_1125916_part3_fix_tests_v2.patch

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

I browsed through the changes, it seems nothing changed since Jonas reviewed withing Comment 75. As discussed, we should land those changes and then follow up on the HTMLMediaElement as well as the imgLoader bits. I have seen you already filed those follow up bugs.

::: docshell/base/nsDocShell.cpp
@@ +10714,5 @@
>        }
>        return rv;
>      }
> +
> +    nsCOMPtr<nsILoadInfo> loadInfo = channel->GetLoadInfo();

I think Jonas wanted you to add a comment here as well, see Comment 75, probably worth adding.

::: dom/html/HTMLMediaElement.cpp
@@ +1332,5 @@
>                                nsIChannel::LOAD_CALL_CONTENT_SNIFFERS);
>  
>    NS_ENSURE_SUCCESS(rv,rv);
>  
> +  nsCOMPtr<nsILoadInfo> loadInfo = channel->GetLoadInfo();

Please add a comment that indicates we will fix the workaround within Bug 1264230.

::: image/imgLoader.cpp
@@ +845,5 @@
>    }
>    (*aResult)->SetLoadGroup(loadGroup);
>  
> +  if (callbacks) {
> +    nsCOMPtr<nsILoadContext> loadContext = do_GetInterface(callbacks);

Same here, please add comment to indciate we will fix the workaround within Bug 1264231.
Attachment #8740857 - Flags: review?(ckerschb) → review+
Assignee

Comment 97

3 years ago
Attachment #8740857 - Attachment is obsolete: true
Attachment #8740989 - Flags: review+
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 100

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/23c4b279a2b1
https://hg.mozilla.org/mozilla-central/rev/1324f1d0dcd0
https://hg.mozilla.org/mozilla-central/rev/b8b7fa054965
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

3 years ago
Blocks: 1263496

Updated

3 years ago
No longer blocks: 1263496
You need to log in before you can comment on or make changes to this bug.