1186290 - Notify TabParent to switch process when a signed packaged is loading from different origin.

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

We are implementing process switching for <iframe mozbrowser>

When content side nsDocShell uses ipc (PHttpChannel?) to request the opening of a new URL, the parent side necko should 1) check the signature and decide whether to switch process, 2) notify both the TabParent and TabChild

In order to do that we might need a new callback to indicate that whether the process will be switch or not.

Comment 1

[Henry Chang [:hchang]]

Attachment: WIP patch

Comment 2

[Henry Chang [:hchang]]

Attachment: WIP patch

Comment 3

[Henry Chang [:hchang]]

Discussing with Kanru offline, there's no need to notify HttpChannelChild. HttpChannelParent is good enough to switch the process.

Comment 4

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry] from comment #3)
> Discussing with Kanru offline, there's no need to notify HttpChannelChild.
> HttpChannelParent is good enough to switch the process.

Also, necko will tell TabParent the new origin that TabParent should use.

Comment 5

[Jason Duell]

Comment on attachment 8643046 [details] [diff] [review]
WIP patch

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

If this winds up being the architecture we take here (we're still talking about it in the email thread) then here are my comments on this patch.  

I'd like :mayhemer to have the final review/decision here.

::: netwerk/base/nsIPackagedAppChannelListener.idl
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

vi modeline too, please:

  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line

@@ +5,5 @@
> +
> +#include "nsISupports.idl"
> +
> +[scriptable, uuid(734a5540-3aae-11e5-b970-0800200c9a66)]
> +interface nsIPackagedAppChannelListener : nsISupports

Do we want to make this interface inherit from nsIStreamListener?  That might make sense if all users of it implement both IDLs.

Either way, add this comment above interface:

 /** 
  * Listens for notifications that the requested necko channel will need to be opened in a different process (for security reasons).
  */

@@ +7,5 @@
> +
> +[scriptable, uuid(734a5540-3aae-11e5-b970-0800200c9a66)]
> +interface nsIPackagedAppChannelListener : nsISupports
> +{
> +    void notifySwitchingProcess();

Add this comment:

  /*    Notification that the listeners' channel needs to be opened in a different process.
   *
   *    After this function has been called, NO further function calls
   *    should be made on the channel.  It is a dead object for all purposes.
   *    The reference that the channel holds to the listener in the child is
   *    released is once this function completes, and no other
   *    nsIStreamListener calls (OnStartRequest/OnDataAvailable/OnStopRequest) will be made
   *    by the channel.

One question: should we pass in the nsIRequest (or nsIHttpRequest if we're confident that this will always be HTTP only) as part of the function call?  I.e. will the callee always only have one channel open, or will it need help determining which channel the callback is for?  Obviously nsHttpChannel and HttpChannel{Parent/Child} know which channel we're talking about, but I'm less sure about the HttpChannelChild's listener (docshell?)

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +419,5 @@
> +HttpChannelChild::RecvNotifySwitchingProcess()
> +{
> +  nsCOMPtr<nsIPackagedAppChannelListener> listener = do_QueryInterface(mListener);
> +  if (listener) {
> +    listener->NotifySwitchingProcess();

If for some reason the listener does not QI to PackagedAppListener, we should cancel the channel.  The best way to do that is probably to call AsyncAbort().  Or we could do MOZ_RELEASE_ASSERT(listener), because IIUC we should never ever call SwitchNotifyProcess in a case where the listener is not a nsIPackagedAppChannelListener.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5535,5 @@
> +{
> +    nsCOMPtr<nsIPackagedAppChannelListener> listener = do_QueryInterface(mListener);
> +
> +    if (packageChannelListener) {
> +        listener->NotifySwitchingProcess();

We should AsyncAbort() if mListener fails the QI.

@@ +5538,5 @@
> +    if (packageChannelListener) {
> +        listener->NotifySwitchingProcess();
> +    } 
> +
> +    return NS_OK;

So I assume that we don't want to ever deliver OnStart/Data/Stop after we've called NotifySwitchProcess?  I'm guessing we want to Cancel() the channel here (to stop the underlying socket/cache entry from doing more I/O) and add some sort of new member variable (mProcessSwitched) that when true, causes us to skip calling the listener's OnStart/Stop methods (see my comments in nsIPackagedAppListener.idl).

Comment 6

[Henry Chang [:hchang]]

Thanks for your review, Jason!

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #5)
> Comment on attachment 8643046 [details] [diff] [review]
> WIP patch
> 
> Review of attachment 8643046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If this winds up being the architecture we take here (we're still talking
> about it in the email thread) then here are my comments on this patch.  
> 
> I'd like :mayhemer to have the final review/decision here.
> 
> ::: netwerk/base/nsIPackagedAppChannelListener.idl
> @@ +1,1 @@
> > +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> 
> vi modeline too, please:
> 
>  
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Coding_Style#Mode_Line
> 
> @@ +5,5 @@
> > +
> > +#include "nsISupports.idl"
> > +
> > +[scriptable, uuid(734a5540-3aae-11e5-b970-0800200c9a66)]
> > +interface nsIPackagedAppChannelListener : nsISupports
> 
> Do we want to make this interface inherit from nsIStreamListener?  That
> might make sense if all users of it implement both IDLs.
> 
> Either way, add this comment above interface:
> 
>  /** 
>   * Listens for notifications that the requested necko channel will need to
> be opened in a different process (for security reasons).
>   */
> 
> @@ +7,5 @@
> > +
> > +[scriptable, uuid(734a5540-3aae-11e5-b970-0800200c9a66)]
> > +interface nsIPackagedAppChannelListener : nsISupports
> > +{
> > +    void notifySwitchingProcess();
> 
> Add this comment:
> 
>   /*    Notification that the listeners' channel needs to be opened in a
> different process.
>    *
>    *    After this function has been called, NO further function calls
>    *    should be made on the channel.  It is a dead object for all purposes.
>    *    The reference that the channel holds to the listener in the child is
>    *    released is once this function completes, and no other
>    *    nsIStreamListener calls
> (OnStartRequest/OnDataAvailable/OnStopRequest) will be made
>    *    by the channel.
> 
> One question: should we pass in the nsIRequest (or nsIHttpRequest if we're
> confident that this will always be HTTP only) as part of the function call? 
> I.e. will the callee always only have one channel open, or will it need help
> determining which channel the callback is for?  Obviously nsHttpChannel and
> HttpChannel{Parent/Child} know which channel we're talking about, but I'm
> less sure about the HttpChannelChild's listener (docshell?)
> 
> ::: netwerk/protocol/http/HttpChannelChild.cpp
> @@ +419,5 @@
> > +HttpChannelChild::RecvNotifySwitchingProcess()
> > +{
> > +  nsCOMPtr<nsIPackagedAppChannelListener> listener = do_QueryInterface(mListener);
> > +  if (listener) {
> > +    listener->NotifySwitchingProcess();
> 
> If for some reason the listener does not QI to PackagedAppListener, we
> should cancel the channel.  The best way to do that is probably to call
> AsyncAbort().  Or we could do MOZ_RELEASE_ASSERT(listener), because IIUC we
> should never ever call SwitchNotifyProcess in a case where the listener is
> not a nsIPackagedAppChannelListener.
> 
> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +5535,5 @@
> > +{
> > +    nsCOMPtr<nsIPackagedAppChannelListener> listener = do_QueryInterface(mListener);
> > +
> > +    if (packageChannelListener) {
> > +        listener->NotifySwitchingProcess();
> 
> We should AsyncAbort() if mListener fails the QI.
> 
> @@ +5538,5 @@
> > +    if (packageChannelListener) {
> > +        listener->NotifySwitchingProcess();
> > +    } 
> > +
> > +    return NS_OK;
> 
> So I assume that we don't want to ever deliver OnStart/Data/Stop after we've
> called NotifySwitchProcess?  I'm guessing we want to Cancel() the channel
> here (to stop the underlying socket/cache entry from doing more I/O) and add
> some sort of new member variable (mProcessSwitched) that when true, causes
> us to skip calling the listener's OnStart/Stop methods (see my comments in
> nsIPackagedAppListener.idl).

We will not deliver OnStart/Data/Stop after notifying BUT the innner channel should still be working. The reason is PackagedAppService already handles the concurrent packaged content requests very well. If a package is downloading, another request for the same package content would just wait for its completion (more exactly, wait for the requested resource being cached, not necessary the entire package need to be downloaded). Does it make sense to you?

I will create a new patch for only notifying HttpChannelParent. Thanks!

Comment 7

[Jason Duell]

> We will not deliver OnStart/Data/Stop after notifying BUT the innner channel
> should still be working. The reason is PackagedAppService already handles the
> concurrent packaged content requests very well. If a package is downloading,
> another request for the same package content would just wait for its
> completion (more exactly, wait for the requested resource being cached, not
> necessary the entire package need to be downloaded). Does it make sense to
> you?

Yes, exactly.  We stop the inner channel but the package keeps downloading (and yes, the new request for the same URI in the new child process will wait for the package to download).

By the way, I recommend looking at the logic that we do for Diverted channels (i.e. channels that have had DivertTo() called on them).  We would do something similar for our case (clear the listener, remove the channel from the loadGroup):

  https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp?from=HttpChannelChild.cpp#499

> I will create a new patch for only notifying HttpChannelParent. Thanks!

No need to do that yet?  It's not clear yet whether we want to only notify the parent or also the HttpChannelChild.  Let's figure that out first.

>

Comment 8

[Henry Chang [:hchang]]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #7)
> > We will not deliver OnStart/Data/Stop after notifying BUT the innner channel
> > should still be working. The reason is PackagedAppService already handles the
> > concurrent packaged content requests very well. If a package is downloading,
> > another request for the same package content would just wait for its
> > completion (more exactly, wait for the requested resource being cached, not
> > necessary the entire package need to be downloaded). Does it make sense to
> > you?
> 
> Yes, exactly.  We stop the inner channel but the package keeps downloading
> (and yes, the new request for the same URI in the new child process will
> wait for the package to download).
> 
> By the way, I recommend looking at the logic that we do for Diverted
> channels (i.e. channels that have had DivertTo() called on them).  We would
> do something similar for our case (clear the listener, remove the channel
> from the loadGroup):
> 
>  
> https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> HttpChannelChild.cpp?from=HttpChannelChild.cpp#499
> 

Seems very helpful to our need!

> > I will create a new patch for only notifying HttpChannelParent. Thanks!
> 
> No need to do that yet?  It's not clear yet whether we want to only notify
> the parent or also the HttpChannelChild.  Let's figure that out first.
> 
> >

Thanks for the useful information! Let's figure that out first!

Comment 9

[Henry Chang [:hchang]]

Attachment: Bug1186290.patch (Almost the same, just fixed build error and add "newOrigin" arguement)

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 8644229 [details] [diff] [review]
Bug1186290.patch (Almost the same, just fixed build error and add "newOrigin" arguement)

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

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +417,5 @@
>  
> +bool
> +HttpChannelChild::RecvNotifySwitchingProcess(const nsCString& aNewOrigin)
> +{
> +  nsCOMPtr<nsIPackagedAppChannelListener> listener = do_QueryInterface(mListener);

the listener here is probably the HTML parser, so I don't expect it to impl your new interface anyway.

you may need to query notification callbacks.  you should also be in parity in nsHttpChannel with that.  HttpChannelParentListener is set as notification callbacks on the parent.

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +394,5 @@
>      mResourceList.AppendLiteral(";");
>  
> +    NS_WARNING("------PackagedAppDownloader::OnStopRequest------");
> +    NS_WARNING(uriAsciiSpec.get());
> +    NS_WARNING("------------------------------------------------");

so, where do you plan to call channel->NotifySwitchingProcess?

Comment 11

[Henry Chang [:hchang]]

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #10)
> Comment on attachment 8644229 [details] [diff] [review]
> Bug1186290.patch (Almost the same, just fixed build error and add
> "newOrigin" arguement)
> 
> Review of attachment 8644229 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/HttpChannelChild.cpp
> @@ +417,5 @@
> >  
> > +bool
> > +HttpChannelChild::RecvNotifySwitchingProcess(const nsCString& aNewOrigin)
> > +{
> > +  nsCOMPtr<nsIPackagedAppChannelListener> listener = do_QueryInterface(mListener);
> 
> the listener here is probably the HTML parser, so I don't expect it to impl
> your new interface anyway.
> 
> you may need to query notification callbacks.  you should also be in parity
> in nsHttpChannel with that.  HttpChannelParentListener is set as
> notification callbacks on the parent.
> 

Got it! So helpful info.

> ::: netwerk/protocol/http/PackagedAppService.cpp
> @@ +394,5 @@
> >      mResourceList.AppendLiteral(";");
> >  
> > +    NS_WARNING("------PackagedAppDownloader::OnStopRequest------");
> > +    NS_WARNING(uriAsciiSpec.get());
> > +    NS_WARNING("------------------------------------------------");
> 
> so, where do you plan to call channel->NotifySwitchingProcess?

Supposed to call after PackagedAppService::Downloader detects the resource is from a signed package and the package origin is different from LoadInfo.loadingPrincipal.origin. The notification will tell the new request what the new origin should be so the re-made request will have the same origin as the signed package. Does it make sense to you? Thanks!

Comment 12

[Henry Chang [:hchang]]

Attachment: Bug1186290.patch

Comment 13

[Henry Chang [:hchang]]

The patch adds a new interface nsIPackagedAppChannelListener which provides a callback as a signalling path to notify TabParent that a signed package is about to load. TabParent will determine if it is necessary to use a new process to load the signed content:

If the content is not a top level document (by checking loadInfo.contentPolicyType), there’s no need to switch process.
If the channel’s origin is as same as the loading origin, don’t switch process.
If the channel is loaded by moz-safe-about:[home/blank], don’t switch process.
Otherwise, request frameLoader to load the channel URI in a new process.

When we call nsIPackagedAppService.getResource, the requesting outer channel will be added to a queue by the package’s URL (a PackagedAppDownloader is corresponding to a package’s URL (and some meta info like appId, isInBrowser, …)) Every time the downloader verifies the package manifest and found it’s a signed package, it will notify all the requesters that a signed content is going to load. Besides, whenever the downloader is gonna serve the content to the any of the requester, it would also notify all the requesters OnStartSignedPackageRequest so that we can switch the process as early as possible.

Regarding the package identifier, it will be addressed by Bug 1178526. At the moment we just use the origin as the identifier. Bug 1178526 will extract the package identifier, callback all the way to HttpChannelChild to update the origin.

Comment 14

[Henry Chang [:hchang]]

Comment on attachment 8659181 [details] [diff] [review]
Bug1186290.patch

Hi Jason,

Since you've already had a overview of the approach of this patch before, can I have your feedback first?

Please see the latest comments for what the patch does. I am still thinking if we should call "AsyncAbort" whenever the listener cannot be queried as nsIPackagedAppChannelListener (since I don't know if the test case will use the http channel without proper listener.)

I am writing a test case for this patch. By then I will ask for the formal review if you are available to do it.

Thanks!

Comment 15

[Henry Chang [:hchang]]

Attachment: Bug1186290_testcase.patch

Comment 16

[Henry Chang [:hchang]]

Attachment: Bug1186290_test_case.patch

Comment 17

[Henry Chang [:hchang]]

Hi Honza,

sorry for distract you but I suppose you already have the overview of this bug. Are you available to review the patch as well as the test case? The decision of process switch will be made in TabParent, which I will ask for Kanru's review. Could you review the necko part? Thanks!

Comment 18

[Henry Chang [:hchang]]

Attachment: Bug1186290.patch

Comment 19

[Henry Chang [:hchang]]

Attachment: Bug1186290.patch

Comment 20

[Henry Chang [:hchang]]

Comment on attachment 8661673 [details] [diff] [review]
Bug1186290.patch

Hi Kanru,

Can I have your review on the TabParent part? It's implemented as we discussed before. Thanks :)

Comment 21

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8661673 [details] [diff] [review]
Bug1186290.patch

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

::: dom/ipc/TabParent.cpp
@@ +443,5 @@
> +  // The resource URL we are loading.
> +  nsCOMPtr<nsIURI> uri;
> +  aChannel->GetURI(getter_AddRefs(uri));
> +  nsCString uriString;
> +  uri->GetAsciiSpec(uriString);

Move this block nearer the LOG that uses the data.

@@ +479,5 @@
> +
> +  if (loadingOrigin == resultPrincipalOrigin) {
> +    LOG(("Loading from the same signed package. No need to switch process."));
> +    return false;
> +  }

I think using nsIPrincipal::Equals to check the two principals is better but I could be wrong.

@@ +484,5 @@
> +
> +  if (StringBeginsWith(loadingOrigin, NS_LITERAL_CSTRING("moz-safe-about:"))) {
> +    LOG(("The content is already loaded by a brand new process."));
> +    return false;
> +  }

Please put a comment to explain why this is OK.

@@ +485,5 @@
> +  if (StringBeginsWith(loadingOrigin, NS_LITERAL_CSTRING("moz-safe-about:"))) {
> +    LOG(("The content is already loaded by a brand new process."));
> +    return false;
> +  }
> +

I assume we have already verified the signature up to this point. Could we assert that?

@@ +500,5 @@
> +  nsCOMPtr<nsIURI> uri;
> +  aChannel->GetURI(getter_AddRefs(uri));
> +
> +  // TODO: Use a proper error code.
> +  aChannel->Cancel(NS_ERROR_UNKNOWN_HOST);

nit: fix this before landing

@@ +505,5 @@
> +
> +  nsCString uriString;
> +  uri->GetAsciiSpec(uriString);
> +  LOG(("We decide to switch process. Call TabParent::SwitchProcessAndLoadURIs: %s",
> +       uriString.get()));

FrameLoader::SwitchProcessAndLoadURI

@@ +510,5 @@
> +
> +  nsRefPtr<nsFrameLoader> frameLoader = GetFrameLoader();
> +  if (!frameLoader) {
> +    return;
> +  }

if (NS_WARN_IF(!frameLoader)) ...

Comment 22

[Henry Chang [:hchang]]

Thanks Kanru :)

(In reply to Kan-Ru Chen [:kanru] from comment #21)
> Comment on attachment 8661673 [details] [diff] [review]
> Bug1186290.patch
> 
> Review of attachment 8661673 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/TabParent.cpp
> @@ +443,5 @@
> > +  // The resource URL we are loading.
> > +  nsCOMPtr<nsIURI> uri;
> > +  aChannel->GetURI(getter_AddRefs(uri));
> > +  nsCString uriString;
> > +  uri->GetAsciiSpec(uriString);
> 
> Move this block nearer the LOG that uses the data.
> 

I'd probably remove all the logs here.

> @@ +479,5 @@
> > +
> > +  if (loadingOrigin == resultPrincipalOrigin) {
> > +    LOG(("Loading from the same signed package. No need to switch process."));
> > +    return false;
> > +  }
> 
> I think using nsIPrincipal::Equals to check the two principals is better but
> I could be wrong.
> 

You're right. Comparing origin is equivalent to nsIPrincipal::Equals.

> @@ +484,5 @@
> > +
> > +  if (StringBeginsWith(loadingOrigin, NS_LITERAL_CSTRING("moz-safe-about:"))) {
> > +    LOG(("The content is already loaded by a brand new process."));
> > +    return false;
> > +  }
> 
> Please put a comment to explain why this is OK.
> 
> @@ +485,5 @@
> > +  if (StringBeginsWith(loadingOrigin, NS_LITERAL_CSTRING("moz-safe-about:"))) {
> > +    LOG(("The content is already loaded by a brand new process."));
> > +    return false;
> > +  }
> > +
> 
> I assume we have already verified the signature up to this point. Could we
> assert that?
> 

Asserting the signedPkg(aka 'package identifier') of the channel principal might be okay but we need to wait Bug 1178526.

Thanks!

Comment 23

[Honza Bambas (:mayhemer)]

(In reply to Henry Chang [:henry] from comment #17)
> Hi Honza,
> 
> sorry for distract you but I suppose you already have the overview of this
> bug. Are you available to review the patch as well as the test case? The
> decision of process switch will be made in TabParent, which I will ask for
> Kanru's review. Could you review the necko part? Thanks!

How urgent is it?

Comment 24

[Henry Chang [:hchang]]

I would hope to land this before the work week (almost two weeks from now). Is it okay for you? Thanks :)

Comment 25

[Honza Bambas (:mayhemer)]

Comment on attachment 8661673 [details] [diff] [review]
Bug1186290.patch

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

r=honzab with comments.

I only have some concerns about potential duplication of TabParent::OnStartSignedPackageRequest calls.

I don't know in what all cases the NotifyOnStartSignedPackageRequest can invoke and, more importantly, when.  Your burden to make sure it's invoked for all requesters at the right time, and once only.  Roughly it seems it is, but I haven't verified in-depth.

::: dom/ipc/TabParent.cpp
@@ +441,5 @@
> +TabParent::ShouldSwitchProcess(nsIChannel* aChannel)
> +{
> +  // The resource URL we are loading.
> +  nsCOMPtr<nsIURI> uri;
> +  aChannel->GetURI(getter_AddRefs(uri));

do all of these before their first use

@@ +456,5 @@
> +  // Get the loading origin.
> +  nsCOMPtr<nsIPrincipal> loadingPrincipal;
> +  loadInfo->GetLoadingPrincipal(getter_AddRefs(loadingPrincipal));
> +  nsCString loadingOrigin;
> +  loadingPrincipal->GetOrigin(loadingOrigin);

any error checks?

anyway, I cannot much review this code.  someone from the NSec designers should do it instead.  (r+ from :kanru maybe enough here)

@@ +482,5 @@
> +    return false;
> +  }
> +
> +  if (StringBeginsWith(loadingOrigin, NS_LITERAL_CSTRING("moz-safe-about:"))) {
> +    LOG(("The content is already loaded by a brand new process."));

isn't there some more deterministic way?  doing StringBeginsWith on a origin definitely doesn't sound right to me.

@@ +490,5 @@
> +  return true;
> +}
> +
> +void
> +TabParent::OnStartSignedPackageRequest(nsIChannel* aChannel)

hmm... when there are multiple loads of signed resources happening, cannot this happen to be called multiple times?  once for each resource?

if so, that doesn't seem right.

@@ +500,5 @@
> +  nsCOMPtr<nsIURI> uri;
> +  aChannel->GetURI(getter_AddRefs(uri));
> +
> +  // TODO: Use a proper error code.
> +  aChannel->Cancel(NS_ERROR_UNKNOWN_HOST);

probably NS_BINDING_FAILED

::: dom/ipc/TabParent.h
@@ +442,5 @@
>      virtual PWebBrowserPersistDocumentParent* AllocPWebBrowserPersistDocumentParent(const uint64_t& aOuterWindowID) override;
>      virtual bool DeallocPWebBrowserPersistDocumentParent(PWebBrowserPersistDocumentParent* aActor) override;
>  
> +    // Called by HttpChannelParent. The function will use a new process to
> +    // load the URI that the given channel associated with if necessary.

please check the sentence, doesn't make much sense to me

@@ +486,5 @@
>  
>      void SetHasContentOpener(bool aHasContentOpener);
> +
> +    // Decide whether we have to use a new process to reload the URI the
> +    // given channel associated with.

here it also sounds strange

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +707,5 @@
>  
> +void
> +PackagedAppService::PackagedAppDownloader::AddRequester(nsIChannel* aRequester)
> +{
> +  mRequesters.AppendObject(aRequester);

assert thread wherever you manipulate this array
also, why don't you store directly the nsIPackagedAppChannelListener interface rather?

@@ +863,5 @@
>  
>  void
>  PackagedAppService::PackagedAppDownloader::NotifyOnStartSignedPackageRequest(const nsACString& aPackageOrigin)
>  {
> +  LOG(("Ready to notify OnStartSignedPackageRequest to all requesters."));

assert thread

@@ +866,5 @@
>  {
> +  LOG(("Ready to notify OnStartSignedPackageRequest to all requesters."));
> +  // Notify all requesters that a signed package is about to download and let
> +  // TabParent to decide if the request needs to be re-made in a new process.
> +  for (uint32_t i = 0; i < mRequesters.Length(); i++) {

for (auto requester : mRequesters) ?

@@ +947,5 @@
> +  // possible whenever needed.
> +  if (mVerifier->GetIsPackageSigned()) {
> +    // TODO: Bug 1178526 will deal with the package identifier things.
> +    //       For now we just use the origin as the identifier.
> +    NotifyOnStartSignedPackageRequest(mVerifier->GetPackageOrigin());

why are you notifying from here as well?  isn't the manifest notification enough?

the "Always notify.." comment doesn't explain WHY you are doing this ;)

::: netwerk/protocol/http/PackagedAppService.h
@@ +123,5 @@
>      // Registers a callback which gets called when the given nsIURI is downloaded
>      // aURI is the full URI of a subresource, composed of packageURI + !// + subresourcePath
> +    nsresult AddCallback(nsIURI *aURI,
> +                         nsICacheEntryOpenCallback *aCallback,
> +                         nsIChannel* aRequester);

comment, who is aRequester

@@ +141,5 @@
>    private:
>      ~PackagedAppDownloader() { }
>  
> +    void AddRequester(nsIChannel* aRequester);
> +    bool RemoveRequester(nsIChannel* aRequester);

again here, add comments!

::: netwerk/protocol/http/PackagedAppVerifier.h
@@ +81,5 @@
> +  {
> +    return mIsPackageSigned;
> +  }
> +
> +  nsCString GetPackageOrigin() const

nsACString const &

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5642,5 @@
> +
> +    if (listener) {
> +        listener->OnStartSignedPackageRequest(aPackageId);
> +    } else {
> +        LOG(("No packaged app channel listener to notify (%p)", mListener.get()));

LOG(("nsHttpChannel::OnStartSignedPackageRequest [this=%p], no listener on %p", this, mListener.get()));

Comment 26

[Honza Bambas (:mayhemer)]

Comment on attachment 8661249 [details] [diff] [review]
Bug1186290_test_case.patch

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

I think this doesn't cover the multiple calls concern I had, but at least does a basic exercise.

::: netwerk/test/unit/test_packaged_app_service.js
@@ +356,5 @@
> +
> +function signedPackagedAppContentHandler(metadata, response)
> +{
> +  response.setHeader("Content-Type", 'application/package');
> +  var body = testData.packageHeader + testData.getData();

the "packageHeader" string is put the body?  not as a response header?

@@ +408,5 @@
> +  packagePath = "/package";
> +  let url = uri + packagePath + "!//index.html";
> +  let channel = getChannelForURL(url, {
> +    onStartSignedPackageRequest: function(aPackageId) {
> +      ok(false, "Unsigned package shouldn't be callback'ed");

"onStartSignedPackageRequest shoudn't be called"

Comment 27

[Henry Chang [:hchang]]

Really thanks for the review! 

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #25)
> Comment on attachment 8661673 [details] [diff] [review]
> Bug1186290.patch
> 
> Review of attachment 8661673 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=honzab with comments.
> 
> I only have some concerns about potential duplication of
> TabParent::OnStartSignedPackageRequest calls.
> 
> I don't know in what all cases the NotifyOnStartSignedPackageRequest can
> invoke and, more importantly, when.  Your burden to make sure it's invoked
> for all requesters at the right time, and once only.  Roughly it seems it
> is, but I haven't verified in-depth.
> 

Every requester can be guaranteed to be notified at most once since we remove the requester from the downloader every notifying. However, it's possible to notify more than once (and it's wrong) "process switch" for a page load. For example, before I find out we can use the ContentPoilcyType to tell the top level document and the subresource apart, a page load might trigger a couple of process switch, which is a mess.

> ::: dom/ipc/TabParent.cpp
> @@ +441,5 @@
> > +TabParent::ShouldSwitchProcess(nsIChannel* aChannel)
> > +{
> > +  // The resource URL we are loading.
> > +  nsCOMPtr<nsIURI> uri;
> > +  aChannel->GetURI(getter_AddRefs(uri));
> 
> do all of these before their first use
> 
> @@ +456,5 @@
> > +  // Get the loading origin.
> > +  nsCOMPtr<nsIPrincipal> loadingPrincipal;
> > +  loadInfo->GetLoadingPrincipal(getter_AddRefs(loadingPrincipal));
> > +  nsCString loadingOrigin;
> > +  loadingPrincipal->GetOrigin(loadingOrigin);
> 
> any error checks?
> 
> anyway, I cannot much review this code.  someone from the NSec designers
> should do it instead.  (r+ from :kanru maybe enough here)
> 

Yes. I will ask Kanru to review TabParent changes :)

> @@ +482,5 @@
> > +    return false;
> > +  }
> > +
> > +  if (StringBeginsWith(loadingOrigin, NS_LITERAL_CSTRING("moz-safe-about:"))) {
> > +    LOG(("The content is already loaded by a brand new process."));
> 
> isn't there some more deterministic way?  doing StringBeginsWith on a origin
> definitely doesn't sound right to me.
> 

How about checking whether the loading principal is null principal? The purpose of this check is to detect if the page is loaded from the new process after we decide to switch process.

> @@ +490,5 @@
> > +  return true;
> > +}
> > +
> > +void
> > +TabParent::OnStartSignedPackageRequest(nsIChannel* aChannel)
> 
> hmm... when there are multiple loads of signed resources happening, cannot
> this happen to be called multiple times?  once for each resource?
> 
> if so, that doesn't seem right.
> 

If it's a subresource load, the ShouldSwitchProcess will return true because its ContentPolicyType is not top level document.

> 
> @@ +866,5 @@
> >  {
> > +  LOG(("Ready to notify OnStartSignedPackageRequest to all requesters."));
> > +  // Notify all requesters that a signed package is about to download and let
> > +  // TabParent to decide if the request needs to be re-made in a new process.
> > +  for (uint32_t i = 0; i < mRequesters.Length(); i++) {
> 
> for (auto requester : mRequesters) ?
> 

I didn't know if our array type supports range-based loop. Will check it.

> @@ +947,5 @@
> > +  // possible whenever needed.
> > +  if (mVerifier->GetIsPackageSigned()) {
> > +    // TODO: Bug 1178526 will deal with the package identifier things.
> > +    //       For now we just use the origin as the identifier.
> > +    NotifyOnStartSignedPackageRequest(mVerifier->GetPackageOrigin());
> 
> why are you notifying from here as well?  isn't the manifest notification
> enough?
> 
> the "Always notify.." comment doesn't explain WHY you are doing this ;)
> 

The case is: when a package is downloading, we request for a resource which is still not cached yet. We can actually wait until the requested resource is seen and notify to switch process. Instead of notifying at the exact time, I choose to notify any waiting requests to switch process earlier. The only downside here is the requested resource may not exist and we waste a process switch effort.

Comment 28

[Henry Chang [:hchang]]

Attachment: Bug1186290_r2.diff

Comment 29

[Henry Chang [:hchang]]

Attachment: Bug1186290_r2.diff

Comment 30

[Henry Chang [:hchang]]

Attachment: Bug1186290_r2.diff

Comment 31

[Henry Chang [:hchang]]

Comment on attachment 8667363 [details] [diff] [review]
Bug1186290_r2.diff

Hi Kanru,

Could you please have a review for the TabParent and the newly added mochitest case "test_signed_web_packaged_app.html"? The test case will create a remote tab, navigate it to a normal page and then to a signed package. It is supposed to result in two processes created. One for the remote tab and one for the signed package.

The test case only covers the case that navigating from the unsigned content to a signed content. For other use cases like from a signed content to a unsigned content or the load of non-top-level document are not covered.

Thanks :)

Comment 32

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1e4650a1202

Comment 33

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8667363 [details] [diff] [review]
Bug1186290_r2.diff

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

::: dom/ipc/TabParent.cpp
@@ +112,5 @@
> +  #ifdef MOZ_WIDGET_GONK
> +    #define LOG(args...) printf_stderr(##args)
> +  #else
> +    #define LOG(args...) PR_LogPrint(##args)
> +  #endif

We could use printf_stderr everywhere so no need to conditional check MOZ_WIDGET_GONK

@@ +454,5 @@
> +  // process switch, consider that we should switch process.
> +
> +  nsCOMPtr<nsILoadInfo> loadInfo;
> +  aChannel->GetLoadInfo(getter_AddRefs(loadInfo));
> +  if (NS_WARN_IF(!loadInfo)) { return true; }

NS_ENSURE_TRUE or keep the return on its own line

@@ +459,5 @@
> +
> +  // Get the loading origin.
> +  nsCOMPtr<nsIPrincipal> loadingPrincipal;
> +  loadInfo->GetLoadingPrincipal(getter_AddRefs(loadingPrincipal));
> +  if (NS_WARN_IF(!loadingPrincipal)) { return true; }

Ditto

@@ +479,5 @@
> +    GetChannelResultPrincipal(aChannel, getter_AddRefs(resultPrincipal));
> +
> +  nsCString resultPrincipalOrigin;
> +  resultPrincipal->GetOrigin(resultPrincipalOrigin);
> +  LOG("Result principal origin: %s", resultPrincipalOrigin.get());

Move above initializations before first use

@@ +499,5 @@
> +  // If this is a brand new process created to load the signed package 
> +  // (triggered by previous OnStartSignedPackageRequest), the loading origin 
> +  // will be "moz-safe-about:blank". In that case, we don't need to switch process
> +  // again. We compare with "moz-safe-about:blank" without appId/isBrowserElement/etc
> +  // taken into account. That's why we use originNoSuffix.

Nit: trailing white spaces

@@ +525,5 @@
> +
> +  nsCString uriString;
> +  uri->GetAsciiSpec(uriString);
> +  LOG("We decide to switch process. Call TabParent::SwitchProcessAndLoadURIs: %s",
> +       uriString.get());

It's nsFrameLoader::SwitchProcessAndLoadURI

@@ +528,5 @@
> +  LOG("We decide to switch process. Call TabParent::SwitchProcessAndLoadURIs: %s",
> +       uriString.get());
> +
> +  nsRefPtr<nsFrameLoader> frameLoader = GetFrameLoader();
> +  if (NS_WARN_IF(!frameLoader)) { return; }

I prefer to keep return on its own line.

@@ +530,5 @@
> +
> +  nsRefPtr<nsFrameLoader> frameLoader = GetFrameLoader();
> +  if (NS_WARN_IF(!frameLoader)) { return; }
> +
> +  frameLoader->SwitchProcessAndLoadURI(uri);

Check the return value and print some warning if it failes.

Comment 34

[Henry Chang [:hchang]]

Thanks for the review, Kanru!

(In reply to Kan-Ru Chen [:kanru] (UTC-7) from comment #33)
> Comment on attachment 8667363 [details] [diff] [review]
> Bug1186290_r2.diff
> 
> Review of attachment 8667363 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/TabParent.cpp
> @@ +112,5 @@
> > +  #ifdef MOZ_WIDGET_GONK
> > +    #define LOG(args...) printf_stderr(##args)
> > +  #else
> > +    #define LOG(args...) PR_LogPrint(##args)
> > +  #endif
> 
> We could use printf_stderr everywhere so no need to conditional check
> MOZ_WIDGET_GONK
>

The thing is printf_stderr behaves a little different on desktop and gonk.
On gonk, it will be redirected to android_printf and a new line will be added
no matter what. That's why I use PR_LogPrint on non-gonk instead so that the
log will always look friendly.

> 
> @@ +479,5 @@
> > +    GetChannelResultPrincipal(aChannel, getter_AddRefs(resultPrincipal));
> > +
> > +  nsCString resultPrincipalOrigin;
> > +  resultPrincipal->GetOrigin(resultPrincipalOrigin);
> > +  LOG("Result principal origin: %s", resultPrincipalOrigin.get());
> 
> Move above initializations before first use
>

Actually it's its first and only use for logging :p
I want it to be logged before any potential subsequent early return.

Comment 35

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6afad5e0ed9

Comment 36

[Henry Chang [:hchang]]

Attachment: Bug1186290_r3.diff

Comment 37

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70270e8e418c

Comment 38

[Henry Chang [:hchang]]

Attachment: Bug1186290_r3.patch

Comment 39

[Henry Chang [:hchang]]

Attachment: Bug1186290_r3.patch (necko part r+'d by honzab)

Comment 40

[Henry Chang [:hchang]]

Thanks Kanru!

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca240e275047

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ca240e275047