756717 - Implement "appcache jars" for apps

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

See bug 756644 for a overall description of what we're needing to do here.

This bug is specifically for implementing the ability to all the appcache to be "per app" rather than just per site. I.e. we should be able to key app-cache on "app" in addition the just the manifest url we're currently keying on.

The most basic use-case is if you install two browsers in Firefox and AwesomeBrowser on a B2G device. If you use Firefox to browse to a website which uses appcache, and then switch to AwesomeBrowser to the same website, we will use different cookies when browsing with AwesomeBrowser, hence we should also use a different appcache.

Comment 1

[Jason Duell]

Honza, if you're ready to take this you'll want to apply my patches for bugs 775119, 775860, and 775861 (in that order) so you can grab the appId and isInBrowserElement from channel's callbacks (nsILoadContext).  I'll add NS_GetAppId and NS_GetInBrowserElement methods to nsNetUtil.h in a sec.   Also, assuming nsHttpChannel winds up using these values in multiple places, it might be worth caching them in HttpBaseChannel::SetNotificationCallbacks (like I already do for mPrivateBrowsing in my patch).

Comment 2

[Jason Duell]

Sicking,

My understanding is that we want a separate appcache for each {appID, isInBrowserElement, origin} combination (where appId=0 means use the default global appcache--i.e. for desktop: appId=0 means we use the existing appcache).  Correct?

Comment 3

[Jason Duell]

per discussion with jonas, I'm going to grab nsScriptSecurityManager::GetExtendedOrigin and put it in nsNetUtil.h, and that will generate a string that we'll use as the per-appcache id.  Patch coming...

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Honza: Any chance you can take this? Sorry that communication appears to have gotten messed up somewhere. This is really the highest priority appcache bug for B2G since appcache won't work on B2G at all without this bug.

I've poked Josh to clarify the priority for this, not sure where communication failed at last attempt :(

Comment 5

[Jason Duell]

So GetExtendedOrigin() in nsILoadContext returns the key for namespaces for appcache/localstorage/IndexedDB.  I've added a convenience method NS_GetExtendedOrigin in nsNetUtil to get it in my patch for bug 776176.

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: wip1

First draft, basic set of offline cache tests for the usual web content is passing, so at least it seems nothing is broken with this patch.

Feel free to test manually, but I don't guarantee any functionality now.  

I'll write tests.  What is the best test to inspire with?

Comment 7

[Honza Bambas (:mayhemer)]

I have updated this patch locally.  I wanted to a) save implementation of nsILoadContext in offline cache update parent and b) prevent use of ssm in necko/uriloader by turning the app cache apis to use directly the extendedOrigin string.  But thanks heaviness of usage of such api it doesn't seem to me as a good way.

I'd like to let app cache service take nsILoadContext and work with it directly rather then with some final ID that consumer has to correctly generate.  "Generate" here means to let that ID be empty when origin == extendedOrigin, that is currently impossible to detect in a convenient way and w/o dep on ssm interface (see [1] and "GetJARIdentifier" function).  I am deciding whether origin == extendedOrigin on too many places wide in the code (in a quick and very hacky way just to check the appcache code works) in the updated patch.  So really not a way to go.

Thus I'm completely blocked on how bug 777720 is about to evolve.  If we add some way to recognize extended origin from origin on that interface, then I'm happy.


[1] https://bugzilla.mozilla.org/attachment.cgi?id=645778&action=diff#a/netwerk/cache/nsDiskCacheDeviceSQL.cpp_sec2

PS: I cannot decide whether extended origin is just origin by creating and comparing origin of e.g. manifestURL (or some same-origin URI where available) again w/o ssm.

Comment 8

[Jason Duell]

If you keep and store a LoadContextParent (from bug 777419) you can keep an nsILoadGroup around in the parent (or child), and ask it whenever you want for extendedOrigin and it will generate it for you (it lives in /docshell so no issue about calling ssm from /necko).  Is that good enough?

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell (:jduell) from comment #8)
> If you keep and store a LoadContextParent (from bug 777419) you can keep an
> nsILoadGroup around in the parent (or child), and ask it whenever you want
> for extendedOrigin and it will generate it for you (it lives in /docshell so
> no issue about calling ssm from /necko).  Is that good enough?

Jason, this absolutely doesn't address my concern.

Yes, it is good, that we now can easily have load context on the parent, good job!  But my concern is different: how to recognize that extendedOrigin is extended w/o ssm.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, and keep in mind that we need to be able to delete data on a per-appid basis when sn app is uninstalled.

Comment 11

[Honza Bambas (:mayhemer)]

If we have a mapping of app to appcache manifest url (AFAIK we do), then it is doable with what I'm locally tuning.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That's not enough. We're not just talking about the appcache used to cache the app itself.

For example the B2G Firefox app can allow several websites to be cached just as they are in Desktop firefox. These websites can have arbitrary URLs, however they will all have the Firefox appId.

If the user uninstalls Firefox, we want to remove all data associated with Firefox (since it's useless as it can't be reached from other apps). Hence we want to remove all appcached data with the Firefox appid.

This doesn't just apply to Firefox of course, but to any browser app installed by the user.

Comment 13

[Jason Duell]

Jonas,  what's our design for uninstall notifications?  A new nsIObserverService notification?  Is it implemented/documented anywhere?

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We don't have anything designed yet. But yes, I think we should do it as a nsIObserverService notification.

Comment 15

[Honza Bambas (:mayhemer)]

Jonas, OK.  Then I'm not quit sure how this is going to work.  Say we have:

<iframe mozbrowser src="browser-URL">
  <iframe mozapp src="app-page-URL">
    <html manifest="manifestURL">

Then the app at app-page-URL that invokes the app cache update will have isInMozBrowser=true and appID=some:app:id.  How can I recognize that the app at app-page-URL is installed by a browser at browser-URL using just load context?

As I understand, the groupID then should be consisted from manifestURL + '#' + [true /*mobrowser*/] + browser-URL + some:app:id.  We can then search for #true+browser-URL to gather all entries belonging to that browser, is that what you mean?

At the moment I don't know hot to get the "browser-URL".

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Honza Bambas (:mayhemer) from comment #15)
> Jonas, OK.  Then I'm not quit sure how this is going to work.  Say we have:
> 
> <iframe mozbrowser src="browser-URL">
>   <iframe mozapp src="app-page-URL">
>     <html manifest="manifestURL">

FWIW, right now this is a theoretical construct. The contents of the toplevel iframe here is a webpage, and webpages doesn't have access to create mozapp iframes. Only very high privileged apps are allowed to do that (essentially only the "window manager" app on B2G).

But assuming that changes at some point in the future...

> Then the app at app-page-URL that invokes the app cache update will have
> isInMozBrowser=true and appID=some:app:id.

Actually no. A <iframe mozapp> always acts like a hard boundary for any behavior. So the page at app-page-URL will have isInMozBrowser=false.

> How can I recognize that the app
> at app-page-URL is installed by a browser at browser-URL using just load
> context?

Which means that you don't need to do this.

> As I understand, the groupID then should be consisted from manifestURL + '#'
> + [true /*mobrowser*/] + browser-URL + some:app:id.

No, the groupdID will be manifestURL + '#' + 'f' + some:app:id

When the app is uninstalled you'd have to delete all appcaches with groupIDs which end with 'f' + some:app:id and 't' + some:app:id.

Comment 17

[Honza Bambas (:mayhemer)]

Attachment: wip2

- nsIApplicationCache.groupID is no longer just the pure manifest URL and cannot be used for that purpose
- based on the previous point, added nsIApplicationCache.manifestURL attribute and used on proper places
- groupID now consist of manifestURL + appid + {f|t}
- there is a new method on nsIApplicationCacheService that builds a groupID from manifestURL and nsILoadContext
- #define NECKO_NO_APP_ID 0 in nsNetUtil.h to avoid using ssm in necko/uriloader
- should update to use new loadcontext implementation from bug 776174 ; can be done later, though

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 647475 [details] [diff] [review]
wip2

https://tbpl.mozilla.org/?tree=Try&rev=910b4643263f

Comment 19

[Thinker Li [:sinker]]

Comment on attachment 647475 [details] [diff] [review]
wip2

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

This patch look good for me.  I just have one opinion.

::: uriloader/prefetch/OfflineCacheUpdateParent.cpp
@@ +201,5 @@
> +OfflineCacheUpdateParent::GetAppId(PRUint32 *aAppId)
> +{
> +    *aAppId = mAppId;
> +    return NS_OK;
> +}

I suggest to move these functions(implementation of nsILoadContext) to class OfflineCacheUpdate. Because, later, we will implement auto-update for WEB Apps.  The updater is running without a window/nsILoadContext.  It will be easier for the updater to implement nsILoadContext at nsOfflineCacheUpdate.

::: uriloader/prefetch/nsOfflineCacheUpdate.h
@@ +319,5 @@
>                        nsIURI *aDocumentURI,
>                        nsIDOMDocument *aDocument,
>                        nsIDOMWindow* aWindow,
>                        nsIFile* aCustomProfileDir,
>                        nsIOfflineCacheUpdate **aUpdate);

We just need to to add another Schedule() to pass AppID & isBrowserElement as args for auto-updater if nsILoadContext been implemented at nsOfflineCacheUpdate.

Comment 20

[Jason Duell]

Comment on attachment 647475 [details] [diff] [review]
wip2

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

::: dom/ipc/PBrowser.ipdl
@@ +208,5 @@
>       *   document will get cached and that we don't re-cache a document that 
>       *   has already been cached (stickDocument=false).
>       */
> +    POfflineCacheUpdate(URI manifestURI, URI documentURI,
> +                        bool isInBrowserElement, PRUint32 appId,

Pass a IPC::LoadContext instead of isInBrowser/appId.  See bug 776174.  

Most of the rest of my comments show where else you would change things to make this work.  In general it will allow you to pass nsILoadContext object (in form of a mozilla::LoadContext object) most places where you've added new appId/InBrowser parameters to functions. (There are many such places so I haven't commented on them all).

I don't object necessarily to landing this code w/o converting to use the LoadContext changes from bug 776174, but we'll need to change eventually, so might as well do it now?

::: netwerk/base/public/nsIApplicationCache.idl
@@ +119,5 @@
>       * cached accordingly. */
>      const unsigned long ITEM_OPPORTUNISTIC = 1 << 6;
>  
>      /**
> +     * URI of the manfiest specifying this application cache.

manifest :)

::: netwerk/base/public/nsNetUtil.h
@@ +1329,5 @@
>      NS_ENSURE_SUCCESS(rv, false);
>      return true;
>  }
>  
> +#define NECKO_NO_APP_ID 0

Add comment saying where this is duplicated from?

::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
@@ +1224,5 @@
> +
> +nsresult
> +GetJARIdentifier(nsIURI *aURI,
> +                 nsILoadContext *aLoadContext,
> +                 nsACString &_result)

What's with the '_result' naming convention?  Better just to use aResult.

::: uriloader/prefetch/OfflineCacheUpdateChild.cpp
@@ +408,5 @@
>      // has not been changed since the last fetch.
>      // See also nsOfflineCacheUpdate::ScheduleImplicit.
>      bool stickDocument = mDocument != nullptr; 
>  
> +    // Carry load context to the parent

If you teach docshell/base/SerializedLoadContext how to get the LoadContext out of a OfflineCacheUpdateChild (use friend or accessor method), you can sKip the marshalling here and just pass IPC::SerializedLoadContext to SendPOfflineCacheUpdateConstructor.

::: uriloader/prefetch/OfflineCacheUpdateParent.cpp
@@ +70,5 @@
>      LOG(("OfflineCacheUpdateParent::RecvSchedule [%p]", this));
>  
> +    // Load context members
> +    mIsInBrowserElement = isInBrowserElement;
> +    mAppId = appId;

Here you can just create a mozilla::LoadContext (mLoadContext) and initialize it from the IPC::LoadContext.

@@ +151,5 @@
>      return NS_OK;
>  }
>  
> +//-----------------------------------------------------------------------------
> +// OfflineCacheUpdateParent::nsILoadContext

If you can hand out the mLoadContext variable I suggest you create (in last comment) whenever this class needs to pretend to be a LoadContext, you can eliminate all these functions?  Not clear to me how this is getting QI'd or whatever to a nsILoadContext--for necko channels it was via GetInterface, so it's easy to just hand off mLoadContext when something asks for nsILoadContext. If this case is similar you can simply avoid having OfflineCacheUpdateParent inherit from nsILoadContext and just hand out in GetInterface.

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to Thinker Li [:sinker] from comment #19)

Thanks.

> I suggest to move these functions(implementation of nsILoadContext) to class
> OfflineCacheUpdate. Because, later, we will implement auto-update for WEB
> Apps.  The updater is running without a window/nsILoadContext.  It will be
> easier for the updater to implement nsILoadContext at nsOfflineCacheUpdate.

Not sure I follow.  Maybe you just have more information, but what you suggest doesn't seem right to me.  But I maybe just need more context on how the autoupdater is going to work.

> We just need to to add another Schedule() to pass AppID & isBrowserElement
> as args for auto-updater if nsILoadContext been implemented at
> nsOfflineCacheUpdate.

Probably yes, or some other mechanism that will pass fake load context.  Also, what you suggest, a bit sounds like we could have some nsIWebAppInfo or so interface that would provide just the two isinbrowser and appid.

(In reply to Jason Duell (:jduell) from comment #20)
> Comment on attachment 647475 [details] [diff] [review]
> wip2

Thanks as well.

> Pass a IPC::LoadContext instead of isInBrowser/appId.  See bug 776174.  

It is a plan (also a comment when adding this attachment).

> If you can hand out ... in
> GetInterface.

It is also a plan.

Guys, thanks for taking a look!  Since I have no more time to update this (I will have to work tomorrow few hours anyway, but probably won't get to this one particular) somebody else has to finish this.  I suggest Thinker to do the updates and let Jason review it.  You two should also talk a bit about the autoupdater, what might be the best land-preparation for it and if it has to be done in this patch/bug (I think a followup is sufficient).

Comment 22

[Jason Duell]

> I suggest Thinker to do the updates and let Jason review it.

That sounds good.  Is that OK with you Thinker?

Comment 23

[Thinker Li [:sinker]]

I can not start this patch until end of this week.  Is it ok?

Comment 24

[Kai-Chih Hu [:khu]]

Is there anyone else can help on this?

Comment 25

[Jason Duell]

Comment on attachment 647475 [details] [diff] [review]
wip2

Thinker,

So do you think  this patch good enough to land for now?  My impression is it's OK: it passes tryserver, and while there are some code cleanups to be done,  nothing is broken.  Is that a correct reading of your review in comment 19?   If so I'd like to land this and then I can code the IPDL cleanup stuff in a followup bug, and I know a reviewer who's fine for that review.

Comment 26

[Jason Duell]

Seems foolhardy to give this (including followups) less than 'M'

Comment 27

[Jason Duell]

Attachment: v3: unbitrotted

unbitrotted stdint and other things.

Still passes try, ready to land AFAICT but holding off until bug 786835 lands, as same issue that blocks cookie-jars from landing (need same necko namespace but currently using inBrowserElement) would break appcache too.

Comment 28

[Jason Duell]

Attachment: v4: v3, unbitrotted, but now breaks mochitests :(

The only semantic change here is the nsILoadContext now has a GetTopFrameElement method.  Apparently returning NS_ERROR_UNIMPLEMENTED isn't enough...

   https://tbpl.mozilla.org/?tree=Try&rev=c447fdbe34ce

Comment 29

[Honza Bambas (:mayhemer)]

Attachment: v4.1

The only failure the v4 of this patch was causing was failure of one check in test_offlineMode.html.  A day or two before we pushed v4 patch to try nsIOService.offline attribute meaning has slightly changed in bug 87717 and some offline cache update when nsIOService.offline = true simply didn't fail as expected.

The offlineMode test was enabled just by mistake in the patch, normally is disabled.

v4.1 - v4 = leave offlineMode test disabled.

Comment 30

[Jason Duell]

Comment on attachment 663080 [details] [diff] [review]
v4.1

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

So previous versions of this patch re-enabled test_offlineMode.html, which is now broken by the change to offline-localhost behavior.  So landing w/o enabling the test leaves us with the status quo here.

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Is this ready to land?

Comment 32

[Jason Duell]

Attachment: v5: rebased

Bug 792036 bitrotted the tests here.  Rebased and pushed to try:

  https://tbpl.mozilla.org/?tree=Try&rev=cc5d34e0e98f

IIUC if this passes try this should land, but I'd like Honza to verify that.

Comment 33

[Honza Bambas (:mayhemer)]

Comment on attachment 664165 [details] [diff] [review]
v5: rebased

https://hg.mozilla.org/integration/mozilla-inbound/rev/293dee8a857a

Comment 34

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/293dee8a857a

Comment 35

[Jason Smith [:jsmith]]

Hi Honza,

You need manual testing support on this? I can help here if need be (I'm assigned to help with any data jars stuff if need be).

Comment 36

[Honza Bambas (:mayhemer)]

Jason, thanks.  Definitely a good thing to test manually now.  I think Jason Duell has some trouble with app cache in some other bug:

(In reply to Jason Duell (:jduell) from bug 786299 comment #14)
> The bad news:  I'm not seeing any entries in the appcache (sqlite db is
> empty, no cached files in profile/OfflineCache) when the app is installed :(

Please provide some help to him in the first line.  Thank you.

Comment 37

[Jason Duell]

Needs test coverage.

Comment 38

[Jason Smith [:jsmith]]

(In reply to Honza Bambas (:mayhemer) from comment #36)
> Jason, thanks.  Definitely a good thing to test manually now.  I think Jason
> Duell has some trouble with app cache in some other bug:
> 
> (In reply to Jason Duell (:jduell) from bug 786299 comment #14)
> > The bad news:  I'm not seeing any entries in the appcache (sqlite db is
> > empty, no cached files in profile/OfflineCache) when the app is installed :(
> 
> Please provide some help to him in the first line.  Thank you.

Sorry for taking so long to get back to you on this. 

I tried installing a web app that preloads the appcache here - http://people.mozilla.com/~fdesre/openwebapps/test.html and then cut the network. Then, I launched the app. The contents loaded successfully.

Smoke tested this feature with the following cases:

PASS - Install an app that preloads the appcache - verify that the app loads on launch without a network connection
PASS -  Go a site in the browser that populates the appcache for the web content. Then, install an app that preloads the appcache that references that same web content. Then, uninstall the app. Verify that going to the web content again in the browser without a network connection should still be viewable.
FAIL - Install an app that preloads the appcache for content off of X site in the browser. Then, cut the network connection. Now, go to that content in the browser. Verify that an error is reported that the content is offline.

The last test didn't pass for me - as I instead got the content preloaded for a different app to load, which I thought shouldn't work in this case. Did I do something wrong? Or is this a bug?

For reference, the test app I was using is here - http://people.mozilla.com/~fdesre/openwebapps/test.html with "install app with appcache". The relevant URL is http://people.mozilla.com/~fdesre/openwebapps/app.html for the content that's loaded that also makes use of appcache.

Comment 39

[Jason Duell]

I'm tracing through the debugger to figure out what's going on here.

Comment 40

[Honza Bambas (:mayhemer)]

Guess: could bug 796045 fix this?

Comment 41

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #40)
> Guess: could bug 796045 fix this?

Ah!  Late 10 days here :D and missed this bug has already been duped.