794663 - Allow downloading appcache for a specific appid/browserflag

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect, P1)

Description

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

When we are downloading and updating a hosted app, we don't have a window or nsILoadContext which contains the appid/browserflag which is downloaded.

So we need an API like:

nsIOfflineCacheUpdateService.
  scheduleUpdate(manifestURL, documentURL, appid, inbrowser);


This should cause the cache associated with <manifestURL, appid, inbrowser> group to be updated. And all requests should be done using cookies from the <appid, inbrowser> cookie-jar.

Comment 1

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

Oh, another thing I forgot here. After an update has been started, we need to be able to get the nsIOfflineCacheUpdate object for the update. I assume that we'd do that by simply having new scheduleUpdate function return the nsIOfflineCacheUpdate object, like the current scheduleUpdate?

Comment 2

[Andrew Overholt [:overholt]]

Affects installation of third party apps.

Comment 3

[Andrew Overholt [:overholt]]

Honza, can you take this?

Comment 4

[Honza Bambas (:mayhemer)]

Sure, deadline?

Comment 5

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

It's a bit up to you when you have time. It's one of the earliest step in a pretty long dependency list for getting hosted apps working end-to-end, but I don't want to set timelines which are too tight.

Would it be possible to have this week? Or mid next week?

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Jonas Sicking (:sicking) from comment #5)
> Would it be possible to have this week? Or mid next week?

I'll do my best.  I think it is doable.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v1

There are some more API changes needed for this feature.  Tested only locally (mochi+xpc).  Fennec for desktop is completely broken (crashes), so I cannot check the e10s parts :(

Comment 8

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

I'm sorry! The intent was that this was just needed in the parent process. I know we discussed this, but we did so in the other bug so it might not have been clear that it still applied here?
Of course, if it works in child processes too then that is ok. But it's not something we will rely on.

Comment 9

[Honza Bambas (:mayhemer)]

First, I was tired when writing comment 7, so it should better stand as: 

"I made some changes touching e10s that might influence normal existing functionality but I was not able to check I didn't break anything."


Then, to make this (or actually anything about offline cache updates) work also from the child process, I just and only need to reach TabChild to be able to create the child/parent e10s machinery.  We usually do that by QI/GI a dom window given.  There may also be some other way to get it, though, I'm not just aware of it.

Comment 10

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

Awesome! Just wanted to make sure that you knew our requirements as to make sure that you knew which options were available.

Makes perfect sense.

Comment 11

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

Jason: review ping?

Comment 12

[Jason Duell]

Comment on attachment 667275 [details] [diff] [review]
v1

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

Looks good except for one error, I think.

+r (no re-review needed) if you just need to set mAppId/mInBrowser.

::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
@@ -1276,5 @@
>                                                               &mPinned);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> -    mLoadContext = aLoadContext;
> -

hmm, so you're not replacing the setting of mLoadContext with setting mAppId/mInBrowser.  IIUC looks like you need to do that here (and possibly in InitForUpdateCheck?), just as you do in OfflineCacheUpdateChild.   As it is now, you're never setting these variables except in the constructor to default values, and that looks like an error.

::: uriloader/prefetch/nsOfflineCacheUpdateService.cpp
@@ +89,5 @@
> +    if (!aWindow)
> +        return NS_OK;
> +
> +    nsCOMPtr<nsIWebNavigation> webNav = do_GetInterface(aWindow);
> +    nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(webNav);

Do we need the two-step dance here, or can we just do_GetInterface(nsILoadContext) directly?  Sicking tells me he'd be ok with that (assuming it works).  Seems like this is just a matter of special-casing nsGlobalWindow::GetInterface(nsILoadContext) to do the same FORWARD_TO_OUTER trick that it does for many other interfaces.  I've filed bug 804495 for this.  It doesn't need to block this bug.

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 667275 [details] [diff] [review]
v1

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

::: uriloader/prefetch/nsOfflineCacheUpdateService.cpp
@@ +86,5 @@
> +    *aAppId = NECKO_NO_APP_ID;
> +    *aInBrowser = false;
> +
> +    if (!aWindow)
> +        return NS_OK;

{}

@@ +91,5 @@
> +
> +    nsCOMPtr<nsIWebNavigation> webNav = do_GetInterface(aWindow);
> +    nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(webNav);
> +    if (!loadContext)
> +        return NS_OK;

{}

@@ +94,5 @@
> +    if (!loadContext)
> +        return NS_OK;
> +
> +    nsresult rv;
> +    rv = loadContext->GetAppId(aAppId);

nsresult rv = ...

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to :Ms2ger from comment #13)
> > +    nsresult rv;
> > +    rv = loadContext->GetAppId(aAppId);
> 
> nsresult rv = ...

No.  Sorry, I really don't like this styling:
- it hides the definition of rv among lines since rv is usually used many times in a function body
- you need to modify more lines when you want to add some rv = ... code before this line
- you may by accident redefine rv when out of the block and you don't get a compiler error
- it prolongs the line

Comment 15

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Good catch with the missing assignment, seems to be wrong and is the only thing that needs an update in the patch.

Carrying r+.

Comment 16

[Alexandre Poirot [:ochameau]]

I tried to apply it without success on m-c/m-a. (conflicts)

Comment 17

[Jason Duell]

It needs to be applied on top of bug 751754.   Honza generally prefers to land his own stuff (he often has a long queue of dependent patches and landing them out of order will break his queue).  I emailed him to ask if we can land both now--they seem ready.

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 674739 [details] [diff] [review]
v1.1

https://hg.mozilla.org/integration/mozilla-inbound/rev/113d5069e67e

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/113d5069e67e

Comment 20

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

Could someone elaborate on hoe risky this would be to take on mozilla-aurora?

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to Jonas Sicking (:sicking) from comment #20)
> Could someone elaborate on hoe risky this would be to take on mozilla-aurora?

This one is a bit more risky then patch for bug 751754, but still, if it sticks with m-c withing few days, then there is no reason not to land it on m-a as well.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/be0ce9f1a08d