1284579 - Remove nsNetUtilNS_UsePrivateBrowsing

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[James Andreou]

nsNetUtil::GetOriginAttributes could now be used to test if a channel is private with the addition of OriginAttributes.mPrivateBrowsingId

Comment 1

[James Andreou]

Attachment: Bug1284579.patch

Comment 2

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8768104 [details] [diff] [review]
Bug1284579.patch

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

Even though it makes the code a bit more verbose, I guess it's a step in the right direction.
Just a couple of minor issues.

::: netwerk/base/nsNetUtil.cpp
@@ +1267,5 @@
> +
> +    // If we are overriding PB because no load context exists, set the origin attributes
> +    // to have a private browsing flag.
> +    bool isPrivate = false;
> +    if (NS_SUCCEEDED(pbChannel->GetIsChannelPrivate(&isPrivate))) {

Bug: pbChannel isn't guaranteed to be non-null here.
Also, do we need to handle loadContext and loadInfo being null?

@@ +1407,2 @@
>                                                      nullptr,
>                                                      &allowed);

nit: indentation.

Comment 3

[James Andreou]

Bug: pbChannel isn't guaranteed to be non-null here.

Fixed.

Also, do we need to handle loadContext and loadInfo being null?

I don't think we do!

Comment 4

[Jason Duell]

James,

Anything we can do to help get this across the finish line?

Comment 5

[James Andreou]

Once 1283281 lands this can be landed

Comment 6

[Valentin Gosu [:valentin] (he/him)]

James, bug 1283281 has landed. Do you think we can land this bug?
Except rebasing (which is needed), do we need anything else?

Comment 7

[James Andreou]

Bug 1282124 is most likely blocking this due to the assertion changes in imgLoader.cpp. Ehsan do the changes to imgLoader depend on your work?

Other than that, should be good to go after rebasing.

Comment 8

[(no longer active)]

Unfortunately bug 1282124 has been stalled for a long time, and I haven't tested your other patches yet, waiting for that one to land first.  I asked Michael to help with landing these patches when that bug lands, so redirecting the needinfo.

But to answer your question, this needs rebasing and a try push to make sure that things still pass.

Comment 9

[Dave Huseby [:huseby]]

I started review for this bug by first looking at Bug 1282124.  In that bug, I found some stuff that worries me.  That bug was to remove the SEC_FORCE_PRIVATE_BROWSING flag from the security flags passed to channel and loadInfo creation functions.  The problem I have is that while trying to verify that the state is checked correctly in those functions I realized that we have both triggering and loading principals.  Which one should be the source of truth for the origin attributes that we check?  I'm worried that since we're not checking the PB state in the same place where we used to set the SEC_FORCE_PRIVATE_BROWSING flag, that all of the places where that security flags was passed to may not correctly check the OA from the correct principal.  I don't fully grok the difference between the two, so I think we should discuss this ASAP so that I can figure out what the correct course of action is.  I'm certain there are lots of places we can add assertion to this patch.

As for the patch in this bug, the changes are complete and there is no boolean left laying around.  I audited all of the changes and they all seem to reference the OA correctly and the logic matches.  There aren't any places to add assertions.

Comment 10

[(no longer active)]

(In reply to Dave Huseby [:huseby] from comment #9)
> I started review for this bug by first looking at Bug 1282124.  In that bug,
> I found some stuff that worries me.  That bug was to remove the
> SEC_FORCE_PRIVATE_BROWSING flag from the security flags passed to channel
> and loadInfo creation functions.  The problem I have is that while trying to
> verify that the state is checked correctly in those functions I realized
> that we have both triggering and loading principals.  Which one should be
> the source of truth for the origin attributes that we check?

The loading principal, as done in <https://hg.mozilla.org/mozilla-central/rev/31c6da3081bf>.

> I'm worried
> that since we're not checking the PB state in the same place where we used
> to set the SEC_FORCE_PRIVATE_BROWSING flag, that all of the places where
> that security flags was passed to may not correctly check the OA from the
> correct principal.  I don't fully grok the difference between the two,

The loading principal is the principal of a Node or a Document or some such which the load belongs to.  For example, when loading an image, the loading principal is the principal of the <img> Element.

The triggering principal is the principal of the code that triggers the load.  These two are usually the same, however in some cases the code that triggers the load may belong to a different principal than the Node associated with the load.  For example, in the implementation of the loadBindingDocument() API, we use the subject principal as the triggering principal (IOW, the triggering principal is the principal for the code that called loadBindingDocument(), whereas the loading principal is the principal of the document that loadBindingDocument() was called on.

The specific meaning of the triggering principal can depend on the kind of load at hand.

> so I
> think we should discuss this ASAP so that I can figure out what the correct
> course of action is.  I'm certain there are lots of places we can add
> assertion to this patch.

I'm not quite sure what the problem in bug 1282124 is.  Previously we read the flag from the docshell's PB flag.  Now we read it from the OA of the loading principal.  The OA of the loading principal is inherited from the OA of the docshell which has the right flag and we have assertions covering that.

> As for the patch in this bug, the changes are complete and there is no
> boolean left laying around.  I audited all of the changes and they all seem
> to reference the OA correctly and the logic matches.  There aren't any
> places to add assertions.

Yes, this bug just needs someone rebasing the patch and landing it.  Michael was originally going to help with that, but I'm not sure if he's still planning to get to this...

Comment 11

[Dave Huseby [:huseby]]

Thanks Ehsan!

Comment 12

[Nika Layzell [:nika] (ni? for response)]

I don't think I will be able to get around to this anytime soon.

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

Hi Jason,
I found Valentin is on PTO, and Ehsan is busy, so I send r? to you.
If you are not able to review this, can you help to forward r? to the right one? 

Thanks

Comment 14

[Kershaw Chang [:kershaw]]

(In reply to Yoshi Huang[:allstars.chh] from comment #13)
> Created attachment 8861829 [details] [diff] [review]
> Patch.
> 
> Hi Jason,
> I found Valentin is on PTO, and Ehsan is busy, so I send r? to you.
> If you are not able to review this, can you help to forward r? to the right
> one? 
> 
> Thanks

Jason is also on PTO until 5/3.
You might want to ask another one to review.

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

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

::: netwerk/base/nsNetUtil.cpp
@@ +1582,5 @@
> +        // Some channels may not implement nsIPrivateBrowsingChannel
> +        nsCOMPtr<nsILoadContext> loadContext;
> +        NS_QueryNotificationCallbacks(aChannel, loadContext);
> +        isPrivate = loadContext && loadContext->UsePrivateBrowsing();
> +    }

Hi Jason,
I wonder in which cases, we won't have nsIPrivateBrowsingChannel?
I found for some tests we still need loadContext to get the correct PB value.

Another strange problem is that in those tests mentioned above, it can QI to nsIPrivateBrowsing in my local machine, however on try server it cannot.

for example, 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=314308fc88ac4e2b2c0a90315cb0b975b2eaece7

the test browser/components/search/test/browser_private_search_perwindowpb.js failed, because it can't QI to nsIPrivateBrowsingChannel.
However in my local machine it can.

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Jason is also on PTO, so fall back the r? to Ehsan.
Ehsan, can you also check my comment 15?

Thanks

Comment 17

[(no longer active)]

Thanks for working on this, Yoshi!

(In reply to Yoshi Huang[:allstars.chh] from comment #15)
> ::: netwerk/base/nsNetUtil.cpp
> @@ +1582,5 @@
> > +        // Some channels may not implement nsIPrivateBrowsingChannel
> > +        nsCOMPtr<nsILoadContext> loadContext;
> > +        NS_QueryNotificationCallbacks(aChannel, loadContext);
> > +        isPrivate = loadContext && loadContext->UsePrivateBrowsing();
> > +    }
> 
> Hi Jason,
> I wonder in which cases, we won't have nsIPrivateBrowsingChannel?

All channels inheriting from <http://searchfox.org/mozilla-central/rev/ce5ccb6a8ca803271c946ccb8b43b7e7d8b64e7a/netwerk/base/PrivateBrowsingChannel.h#23> implement this interface.  Some channels such as nsViewSourceChannel and WebSocketChannel don't.  There are probably a few others that I'm forgetting.

Note that nsIPrivateBrowsingChannel is only used for overriding the privacy status of a channel (that is, for telling the channel to ignore what its loadContext thinks the privacy state should be.

> I found for some tests we still need loadContext to get the correct PB value.
> 
> Another strange problem is that in those tests mentioned above, it can QI to
> nsIPrivateBrowsing in my local machine, however on try server it cannot.
> 
> for example, 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=314308fc88ac4e2b2c0a90315cb0b975b2eaece7
> 
> the test
> browser/components/search/test/browser_private_search_perwindowpb.js failed,
> because it can't QI to nsIPrivateBrowsingChannel.
> However in my local machine it can.

I'm confused.  I don't think it's possible for the QI to fail on try and succeed on your machine...  But looking at your patch, I think the |MOZ_ASSERT((attrs.mPrivateBrowsingId > 0) == mRespectPrivacy);| assertions in imgLoader.cpp may be wrong.  Let me explain.

In a private browsing browser.xul window, we are using the system principal.  Since the system principal is a singleton, its origin attributes can't be modified, so the OA.mPrivateBrowsingId member would still be 0 eventhough we are in a private window. This is in fact why there is mRespectPrivacy on imgLoader in the first place, otherwise we wouldn't need this extra boolean in the first place.  As far as I can tell that is the assertion that your patch is triggering on the try server.

Am I missing something?

Comment 18

[(no longer active)]

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

Hrm... As I was reviewing this I was reading PrivateBrowsingChannel.h more carefully and I think things have changed here quite a bit since I wrote this code originally, and I am no longer sure if I actually understand all of the semantics properly.

In the beginning (in bug 788275) the intention was for nsIPrivateBrowsingChannel::SetPrivate() was to only use it in exceptional cases.  Indeed back then that function was only ever called in *one* case...  But now I was looking at the code and this function is being called all over the place.  :-(  So merely ripping out NS_UsePrivateBrowsing() and replacing it with reading mPrivateBrowsingId won't work unfortunately, I'm afraid.

I think you need to get a second opinion from a real Necko peer here, but based on my poor understanding of Necko, I think we may need to do something more sophisticated here.  We need to somehow get rid of this extra bit of state that PrivateBrowsingChannel is maintaining first...  I'm gonna send this to Patrick as he is my normal go-to person for all things related to Necko.  :-)

Sorry I couldn't be of more help here...

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #17)
> I'm confused.  I don't think it's possible for the QI to fail on try and
> succeed on your machine...  But looking at your patch, I think the
> |MOZ_ASSERT((attrs.mPrivateBrowsingId > 0) == mRespectPrivacy);| assertions
> in imgLoader.cpp may be wrong.  Let me explain.
> 
> In a private browsing browser.xul window, we are using the system principal.
> Since the system principal is a singleton, its origin attributes can't be
> modified, so the OA.mPrivateBrowsingId member would still be 0 eventhough we
> are in a private window. 

I have different view on this, I agree that System Principal shouldn't have any origin attribute.
However in this case we are talking about the origin attributes on the *channel*, even the loadingPrincipal of the channel is System principal, it could be that Chrome is doing the load on behalf of a content page, for example, favicon loading in bug 1277803.
And this argument should also applies to docshell.

It is confusing to me that if I need to know whether this channel or docshell is in PB mode, I need to check if it's for chrome load first:
if it's chrome, I need to look up somewhere else to get the real PB value, 
otherwise I can read originAttributes.mPrivateBrowsingId to know this.

Also see Jonas' comment when we first added these origin attributes.
https://bugzilla.mozilla.org/show_bug.cgi?id=1209162#c12

"The only thing that the OriginAttributes in the docshell should be used for is to help create the correct OriginAttributes for the documents inside that docshell. This does not mean that we simply copy information from the docshell's OriginAttributes to the document's OriginAttributes. It only means that the docshell's OriginAttributes needs to provide enough information to the PrincipalOriginAttributes::InheritFromDocShellToDoc function that it can create the OriginAttributes values that match the ones in the picture."

Perhaps we could keep the OA in the channel/docshell, instead of forcing it to 0 when it's for chrome load.

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #17)
> I'm confused.  I don't think it's possible for the QI to fail on try and
> succeed on your machine...  
I think I know why now.

When we try to load a chrome resource, chrome://... 
in my local machine it would be converted to file://..., which would be a nsFileChannel, and implements nsIPBChannel.

However on try, it would be converted to jar::file://..., which would be a nsJARChannel, and it doesn't implement nsIPBChannel.

Again we still clearly define how origin attributes work on those chrome channels/docshells.
Thanks

Comment 21

[Patrick McManus [:mcmanus]]

I'm not the expert here either.. so I'll chime in, but in the end I'm just going to transfer the review back to valentin who I believe returns on monday (and this doesn't seem particularly more urgent than that)

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #18)
> Comment on attachment 8861829 [details] [diff] [review]
> Patch.
> 
> Hrm... As I was reviewing this I was reading PrivateBrowsingChannel.h more
> carefully and I think things have changed here quite a bit since I wrote
> this code originally, and I am no longer sure if I actually understand all
> of the semantics properly.
> 
> In the beginning (in bug 788275) the intention was for
> nsIPrivateBrowsingChannel::SetPrivate() was to only use it in exceptional
> cases.  Indeed back then that function was only ever called in *one* case...
> But now I was looking at the code and this function is being called all over
> the place.  :-(  So merely ripping out NS_UsePrivateBrowsing() and replacing
> it with reading mPrivateBrowsingId won't work unfortunately, I'm afraid.
> 
> I think you need to get a second opinion from a real Necko peer here, but
> based on my poor understanding of Necko, I think we may need to do something
> more sophisticated here.  We need to somehow get rid of this extra bit of
> state that PrivateBrowsingChannel is maintaining first...  I'm gonna send
> this to Patrick as he is my normal go-to person for all things related to
> Necko.  :-)
> 
> Sorry I couldn't be of more help here...

my read of the code matches ehsan's.. there seem to be quite a few SetPrivate()'s and I'm not really sure whether all the OriginAttributes line up or not to make that irrelevant. 

You might be able to slip the "new OA based algorithm" as an additional check into NS_UsePrivateBrowsing() and assert that the new and old algorithms give the same result.. at least run it through try, but nightly might be interesting too.

Comment 22

[Patrick McManus [:mcmanus]]

and it might be a much smaller patch anyhow if you just replaced the guts of NS_UsePrivateBrowsing() with the new code rather than changing every callsite.

Comment 23

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Patrick McManus [:mcmanus] from comment #22)
> and it might be a much smaller patch anyhow if you just replaced the guts of
> NS_UsePrivateBrowsing() with the new code rather than changing every
> callsite.

I agree, but since Valentin didn't mention this in his last review, I thought the purpose is to remove that function, but if Valentin'd like to change his thoughts, I'll do it.

Comment 24

[Valentin Gosu [:valentin] (he/him)]

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

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

The patch looks quite well overall, but I agree with Patrick that it would be simplified if we just move the logic to NS_UsePrivateBrowsing.
Sorry I didn't say this the first time.

Comment 25

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: revise NS_UsePrivateBrowsing to get PB from origin attributes.

Comment 26

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: revise NS_ShouldCheckAppCache

Comment 27

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: remove IsPrivate arg from nsCookieService.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e263be331753
Part 1:revise NS_UsePrivateBrowsing to get PB from origin attributes. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/323312e3937e
Part 2: revise NS_ShouldCheckAppCache. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/731dd04cdcef
Part 3: remove IsPrivate arg from nsCookieService.  r=valentin

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e263be331753
https://hg.mozilla.org/mozilla-central/rev/323312e3937e
https://hg.mozilla.org/mozilla-central/rev/731dd04cdcef