1195713 - [PackagedAppService] The package cache doesn't use correct nsILoadContextInfo

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Henry Chang [:hchang]]

For now the package cache (not the resource cache) always use a null LoadContextInfo as the cache key because the notification callbacks of the inner channel is not set. It results in the package cache file and the resource cache file uses different nsLoadContextInfo.

Comment 1

[Henry Chang [:hchang]]

Attachment: Bug1195713.patch

Comment 2

[Henry Chang [:hchang]]

Comment on attachment 8649233 [details] [diff] [review]
Bug1195713.patch

Hi Valentin,

Can I have your review on this patch? Currently the inner channel doesn't have a proper notification callback to generate a cache key (nsILoadContextInfo). That's why the package cache always use a null nsILoadContextInfo. Thanks!

Comment 3

[Henry Chang [:hchang]]

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

Comment 4

[Honza Bambas (:mayhemer)]

I am really curious whether this is going to do the job you expect..

Comment 5

[Valentin Gosu [:valentin]]

Comment on attachment 8649233 [details] [diff] [review]
Bug1195713.patch

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

Nice catch. Thanks!
r+ with a couple of notes

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +785,5 @@
>      new PackagedAppChannelListener(downloader, mimeConverter);
>  
> +  nsRefPtr<LoadContext> loadContext = new LoadContext(aPrincipal);
> +  channel->SetNotificationCallbacks(loadContext);
> +

Please include mozilla/LoadContext.h
The build will fail otherwise.

Also, please add 
> channel.notificationCallbacks = new LoadContextCallback(0, false, false, false);
to test_packaged_app_channel.js::test_channel()
otherwise the test will fail.

Comment 6

[Henry Chang [:hchang]]

Because the cache storage for xxx.pak and the sub-resources is opened in different place: 

The *.pak cache storage is opened in [1], which uses |GetLoadContextInfo| to get the appId/inBrowser/etc. |GetLoadContextInfo| calls |NS_GetAppInfo| [2] then the notification callbacks of the inner channel will be queried to get the LoadContext [3] to obtain the appId/isBrowserElement/etc. The inner channel is created without a notification callbacks so the LoadContextInfo used to open the cache storage is always a null one.

In the other hand, the cache storage for sub-resources is opened in [4] and it uses the LoadContextInfo we created in the outer channel [5] so it would have the same appId/isInBrowser/etc as the outer channel.

Did I miss something? Thanks!

[1] http://hg.mozilla.org/mozilla-central/file/90d9b7c391d3/netwerk/protocol/http/nsHttpChannel.cpp#l2887
[2] http://hg.mozilla.org/mozilla-central/file/90d9b7c391d3/netwerk/base/LoadContextInfo.cpp#l57
[3] http://hg.mozilla.org/mozilla-central/file/90d9b7c391d3/netwerk/base/nsNetUtil.cpp#l1225
[4] http://hg.mozilla.org/mozilla-central/file/90d9b7c391d3/netwerk/protocol/http/PackagedAppService.cpp#l341
[5] http://hg.mozilla.org/mozilla-central/file/90d9b7c391d3/netwerk/protocol/http/nsHttpChannel.cpp#l5220

Comment 7

[Valentin Gosu [:valentin]]

The test needs to have a load context, because of the way we pass the principal to the packagedAppService.

We call GetURIPrincipal, which ends up calling to GetChannelURIPrincipal in SSM.
https://dxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#407
If there's no loadContext, it returns a principal with UNKNOWN_APP_ID, which then asserts when creating a new LoadContext for that principal.

Comment 8

[Henry Chang [:hchang]]

Thanks for the review! Valentin.

(In reply to Valentin Gosu [:valentin] from comment #7)
> The test needs to have a load context, because of the way we pass the
> principal to the packagedAppService.
> 
> We call GetURIPrincipal, which ends up calling to GetChannelURIPrincipal in
> SSM.
> https://dxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.
> cpp#407
> If there's no loadContext, it returns a principal with UNKNOWN_APP_ID, which
> then asserts when creating a new LoadContext for that principal.

Comment 9

[Henry Chang [:hchang]]

On Desktop, when we load the resource from cache, it will hit the assertion failure case. Figuring out how to fix it....

Comment 10

[Valentin Gosu [:valentin]]

(In reply to Henry Chang [:henry] from comment #9)
> On Desktop, when we load the resource from cache, it will hit the assertion
> failure case. Figuring out how to fix it....

Following bug 1196021 we pass the channel to ::GetResource.
That means you can probably do:
> nsRefPtr<nsIInterfaceRequestor> loadContext;
> aChannel->GetNotificationCallbacks(getter_Addrefs(loadContext));
> if (loadContext) {
>   channel->SetNotificationCallbacks(loadContext);
> }

This should avoid the assertion. If it's possible, please add a unit test for the use case that triggered the assertion.

Comment 11

[Henry Chang [:hchang]]

(In reply to Valentin Gosu [:valentin] from comment #10)
> (In reply to Henry Chang [:henry] from comment #9)
> > On Desktop, when we load the resource from cache, it will hit the assertion
> > failure case. Figuring out how to fix it....
> 
> Following bug 1196021 we pass the channel to ::GetResource.
> That means you can probably do:
> > nsRefPtr<nsIInterfaceRequestor> loadContext;
> > aChannel->GetNotificationCallbacks(getter_Addrefs(loadContext));
> > if (loadContext) {
> >   channel->SetNotificationCallbacks(loadContext);
> > }
> 
> This should avoid the assertion. If it's possible, please add a unit test
> for the use case that triggered the assertion.

After I fixed some other bugs the assertion failure disappeared like a charm. Weird...

Comment 12

[Henry Chang [:hchang]]

Attachment: Bug1195713 (carry r+, followed Valentin's suggestion and added test case for null notification callbacks)

Comment 13

[Henry Chang [:hchang]]

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

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26a558e221b1

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/26a558e221b1