824695 - Fix bustage from requiring etags for packaged apps to keep the phone in a consistent state during download of a packaged app

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Jason Smith [:jsmith]]

Build: 12/26/2012 B2G18
Device: Unagi

Steps:

1. Go to https://marketplace-dev.allizom.org/app/test-webapi-permissions/
2. Select to install the packaged app
3. Confirm installation

Expected:

The downloading app should start and complete successfully.

Actual:

The downloading starts, but eventually will get hung. Stopping and restarting the download will not fix the problem. Additionally, this also occurred when I tried a different packaged app at http://jds2501.github.com/webapi-permissions-tests/ in "Packaged App Test Case 2." Looks like packaged app installation is broken again :(.

Error Console:

12-25 15:07:07.838: E/GeckoConsole(110): [JavaScript Error: "[Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIHttpChannel.getResponseHeader]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: resource://gre/modules/Webapps.jsm :: <TOP_LEVEL> :: line 1838"  data: no]" {file: "resource://gre/modules/Webapps.jsm" line: 1838}]

Comment 2

[Matthias Versen [:Matti]]

From bug 824710: The reporter there thinks that it happens only if the http server doesn't send an ETAG header.

Comment 3

[Julien Wajsberg [:julienw]]

The line which fails was introduced by Bug 820630.

Comment 4

[Etienne Segonzac (:etienne)]

(In reply to Matthias Versen (Matti) from comment #2)
> From bug 824710: The reporter there thinks that it happens only if the http
> server doesn't send an ETAG header.

Confirming, it works as expected when the server sends etags headers.

Comment 5

[Wil Clouser [:clouserw]]

Is this something the marketplace needs to help with?  We send etags with all installs afaik.

Comment 6

[Andrew Overholt [:overholt]]

We discussed this during triage today and decided we'd like all marketplaces to send etag headers since we rely on them so much for update detection.

Will this affect 3rd party marketplaces?

Comment 7

[Jason Smith [:jsmith]]

Disagrew with triage call. Will explain more when i am not typing this on my phone, but in short - this affects installation of a packaged app from anywhere. That requirement stated above sounds unwise. Reopening and renoming.

Comment 8

[Jason Smith [:jsmith]]

Please do not touch the priority field on a component you do not own. Thanks.

Comment 9

[Jason Smith [:jsmith]]

When I am not in transit traveling (probably late tonight) I will provide a detailed writeup on my rationale.

Comment 10

[Julien Wajsberg [:julienw]]

I completely agree with Jason here.

Please don't forget we're talking about open web apps here. The point of open web apps is that we don't need marketplaces any more (even if they're still useful of course). Please don't break the installation of 3rd party webapps.

The Web works with or without ETag header, with or without Last-Modified header (BTW this is Bug 823648), the very point of Firefox OS is to leverage the Web, so please let's fix this bug.

Comment 11

[Wil Clouser [:clouserw]]

(In reply to Jason Smith [:jsmith] from comment #8)
> Please do not touch the priority field on a component you do not own. Thanks.

I'm changing it in accordance with the decision in our meeting today.

Comment 12

[Jason Smith [:jsmith]]

Then i am voiding that decision, as you have not enough context yet to set a priority. And we only set priorities per how b2g triage works. So there is no meaning on your p4 setting.

Comment 13

[Jason Smith [:jsmith]]

One note while i am not at the computer - this bug reproduces on a 12/26 build with both a marketplace dev privileged app as shown in comment 0 and with a packaged app on a random website. So the current argument made by triage is incorrect anyways - the bug is reproducing on marketplace dev which does send etags.

Comment 14

[Julien Wajsberg [:julienw]]

Moreover, this bug leaves the phone is an inconsistent state because we don't get a normal downloaderror event from gecko.

Let's wait until Fabrice returns and gives us a hint on whether this is easy to fix or not. I suspect the needed amount of energy is less than what we've already spent arguing.

Comment 15

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

Updating summary.

As far as I can tell, we now "simply" require servers to support etags if they want to support packaged apps.

We effectively required that previously too since our update mechanism was heavily relying on etags to detect when a package is updated.

I do think that we should improve this in the future. Likely by using if-modified-since headers as well as possibly comparing contents of the minimanifest files (this is what the appcache does). But that's not something that should be a priority for v1.


I don't think etags is a hard requirements for 3rd party app store servers to fulfill. However possibly this will be a bit of a pain for QA when doing tests for packaged apps since it means that you can only test packaged apps by running a server which supports etags.

Comment 16

[Julien Wajsberg [:julienw]]

Jonas, Wil, I fully understand your reasoning. However, as it is now, the phone is left in an inconsistent state if it happens that a user wants to install a packaged app that happens to not send the ETag header.

If you want to support only packaged apps with ETag (which is still wrong imho, but, well, that is a product decision), we need to properly catch this in gecko and send a proper downloaderror to gaia. Now, it's just broken.

About the reasoning, I still feel something is wrong: the error happens after the download is finished. So let's say the user has downloaded a package of several MB, and now, we'll tell him that he can't install this package because the author doesn't send ETag. After he spent several potentially paid megabytes of his allowed download ratio.

That's not acceptable imho.

I'm in favor of requiring ETag for the manifest (mini for packages, normal for hosted) for v1, but here the bug happens for ETag on the archive. I'm not sure we should require ETag for archives. We should just use it if it's here, and that's ok.

Comment 17

[Jason Smith [:jsmith]]

Fixing title - see comment 13. This reproduces on marketplace dev which does send etags. So this bug isn't just about the etag req - its failing generally right now.

Comment 18

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

Thanks for clarifying. This is definitely a blocker then. Sorry about the confusion.

Comment 19

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

Note that we *have* *to* require either etags or last-modified headers. Otherwise we can't detect when a package has been updated. The way things work right now is

1. Download the mini manifest
2. If the mini manifest hasn't changed, we assume that the app hasn't been updated
3. If the mini manifest has changed, we make a HEAD request to the app package to see if it has changed.

If the app package neither supports etags or if-modifed-since then in step three we would have to assume that the app package was updated every time that the mini manifest was updated. This doesn't seem like a good solution and could easily result in users redownloading the same app package multiple times.

This is really bad in countries where data connections are pricy, something that will be the case in many of our launch markets.

This is especially bad since we've spent a lot of effort building a UI which puts the user in control of managing their expenses. It would be a very bad experience for users if we tell them that a new version of an app is available and then the user decides to spend money on downloading that new version, only to realize that it's still the same version.

I agree that what we are building here is "of the web" and that we can't just think of the mozilla marketplace. My argument is that we should still prioritize users over developers and so "all" we are doing here is adding additional requirements on developers. Not breaking the model.


Since we don't support if-modified-since yet, it would make sense to me to throw an error before showing the install dialog if the minimanifest doesn't send an etag. And raising a download error if the app package doesn't supply an etag.

And then in v2 worry about adding support for if-modified-since and other mechanisms that can give developers more flexibility while still providing a good user experience.


It's unclear to me though if doing those two things would fix the problem here or not.

Comment 20

[Jason Smith [:jsmith]]

Clarifying title so that we know what exactly we are blocking on. There's other implications of the etags I'll talk about more - but I'll take that to email.

Comment 21

[Etienne Segonzac (:etienne)]

(In reply to Jason Smith [:jsmith] from comment #17)
> Fixing title - see comment 13. This reproduces on marketplace dev which does
> send etags. So this bug isn't just about the etag req - its failing
> generally right now.

Not so sure about that statement:

~ [ curl -I https://marketplace-dev.allizom.org/telefonica/downloads/file/181065/scriptalertall-your-base-ar-1.zip
HTTP/1.1 200 OK
Server: nginx
X-Backend-Server: dev2.addons.phx1.mozilla.com
Content-Type: application/zip
Date: Fri, 28 Dec 2012 09:46:33 GMT
Accept-Ranges: bytes
Via: Moz-pp-zlb09
Connection: keep-alive
Set-Cookie: lang="en-US\054"; Path=/
Set-Cookie: region=us; Path=/
Last-Modified: Wed, 12 Dec 2012 15:01:25 GMT
Content-Length: 39712


The marketplace-dev *does* send etags for the manifest, but not the zip file, and looks like this is causing the issue.

Comment 22

[Julien Wajsberg [:julienw]]

Jonas> I fully agree with what you're explaining. However afaik now we don't do a HEAD request to get the headers of the package files before downloading it. I believe we should file another bug. Do you think we should use a HEAD request for the mini manifest as well ?


As for the cause this bug, this is very simple. |getResponseHeader| throws an exception if the header is absent (see [1]).

I'll try to come up with a patch soon.

[1] https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsIHttpChannel.idl#191

Comment 23

[Julien Wajsberg [:julienw]]

Attachment: patch v1

This is untested, I think I won't be able to test before a few hours so feel free to take.

Comment 24

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

Yes, I do think we should make a HEAD request if the minimanifest has changed to make sure that there really is a new package available. Please file a bug on that (and ideally attach a patch :) )

Comment 25

[Jason Smith [:jsmith]]

(In reply to Etienne Segonzac (:etienne) from comment #21)
> (In reply to Jason Smith [:jsmith] from comment #17)
> > Fixing title - see comment 13. This reproduces on marketplace dev which does
> > send etags. So this bug isn't just about the etag req - its failing
> > generally right now.
> 
> Not so sure about that statement:
> 
> ~ [ curl -I
> https://marketplace-dev.allizom.org/telefonica/downloads/file/181065/
> scriptalertall-your-base-ar-1.zip
> HTTP/1.1 200 OK
> Server: nginx
> X-Backend-Server: dev2.addons.phx1.mozilla.com
> Content-Type: application/zip
> Date: Fri, 28 Dec 2012 09:46:33 GMT
> Accept-Ranges: bytes
> Via: Moz-pp-zlb09
> Connection: keep-alive
> Set-Cookie: lang="en-US\054"; Path=/
> Set-Cookie: region=us; Path=/
> Last-Modified: Wed, 12 Dec 2012 15:01:25 GMT
> Content-Length: 39712
> 
> 
> The marketplace-dev *does* send etags for the manifest, but not the zip
> file, and looks like this is causing the issue.

That sounds right. I can confirm this still doesn't work on a 12/28 build. So root cause of the problem here then is a marketplace problem.

Since there's already a patch here I'll morph the bug to the patch's purpose. I'll file a new bug on marketplace about zip file etags.

Comment 26

[Jason Smith [:jsmith]]

bug 825209 tracks the root cause of the problem now.

Morphing the bug to what I *think* this patch will eventually target. If we fail to find an etag, we should fail gracefully, and not spin infinitely in the download cycle till the end of time.

Renoming due to the morph. My opinion - I'd at least track this, given the poor UX here and the fact that app developers could hit this. Don't know about blocking.

Comment 27

[Jason Smith [:jsmith]]

Josh - feel free to add your own input from a UX perspective on this problem

Comment 28

[Julien Wajsberg [:julienw]]

In this bug, I want to fix the breakage due to a server not sending etag for the package. This is wrong because the phone is left in an unconsistent state.

This does NOT happen with manifest because we use XHR to download them, and XHR doesn't throw.

Will test my patch now.

Comment 29

[Julien Wajsberg [:julienw]]

Filed Bug 825218 about doing a HEAD request before GETting the package.

Comment 30

[Jason Smith [:jsmith]]

Gotcha. Updated title again to see if I get it right this time...

Comment 31

[Jason Smith [:jsmith]]

Filed bug 825223 about the etag for the mini-manifest.

Comment 32

[Julien Wajsberg [:julienw]]

Attachment: patch v2

This fixes also another bug with ETag support: the original code doesn't call correctly |setRequestHeader| so this would fail as soon as we try to download the file for an update... or the first time if the ETag was specified for preinstalled apps !

Don't know if Fabrice is the best reviewer as I think he's away now.

Comment 33

[Jason Smith [:jsmith]]

Comment on attachment 696322 [details] [diff] [review]
patch v2

I think Fabrice is out. Fernando might be a good backup reviewer.

Comment 34

[Julien Wajsberg [:julienw]]

Fernando is away as well ;)

Comment 35

[Jason Smith [:jsmith]]

Comment on attachment 696322 [details] [diff] [review]
patch v2

Time for reviewer #3! I know Jonas is here. I think...

Comment 36

[Julien Wajsberg [:julienw]]

If this is r+ please go ahead and check this in as I don't have committer access.

Thanks

Comment 37

[Rob Hudson [:robhudson]]

Comment on attachment 696322 [details] [diff] [review]
patch v2

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

::: dom/apps/src/Webapps.jsm
@@ +1863,5 @@
>            }
>  
>            // Save the new Etag for the package.
> +          try {
> +            app.packageEtag = requestChannel.getResponseHeader("Etag");

Does the casing matter? Viewing the headers from the server this is "ETag" and not "Etag".

Comment 38

[Julien Wajsberg [:julienw]]

This is case insensitive (see [1], and I've seen this work). Do you want that we use a lower-case string to make it more obvious ?

[1] https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsIHttpChannel.idl#183

Comment 39

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

Comment on attachment 696322 [details] [diff] [review]
patch v2

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

Please also put a call in onStartRequest which calls checks for an etag and calls channel->Cancel() if there isn't an etag available.

r=me with that fixed.

::: dom/apps/src/Webapps.jsm
@@ +1863,5 @@
>            }
>  
>            // Save the new Etag for the package.
> +          try {
> +            app.packageEtag = requestChannel.getResponseHeader("Etag");

The case doesn't seem to matter.

Comment 40

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

This is a bad experience, but it's basically "just" error handling. Under normal circumstances users won't be noticing this problem. So minusing for v1 but tracking for v1.x

Comment 41

[Julien Wajsberg [:julienw]]

(In reply to Jonas Sicking (:sicking) from comment #39)
> 
> Please also put a call in onStartRequest which calls checks for an etag and
> calls channel->Cancel() if there isn't an etag available.
> 

Sorry but I don't understand the point of this. Do you want to abort if we have no saved ETag in the app object ? I'm afraid this might break our dogfood users because until recently we didn't have a saved ETag.

Comment 42

[Julien Wajsberg [:julienw]]

(In reply to Jonas Sicking (:sicking) from comment #40)
> This is a bad experience, but it's basically "just" error handling. Under
> normal circumstances users won't be noticing this problem. So minusing for
> v1 but tracking for v1.x

As I said in the comment with the patch, it also fixes a lot more serious issue that would make an update fails as soon as we save an ETag, so I'm renominating.

This is the first chunk in the patch, we're calling "setRequestHeader" with only 2 arguments whereas it requires 3 arguments, and this throws an exception.

Comment 43

[Jason Smith [:jsmith]]

(In reply to Jonas Sicking (:sicking) from comment #40)
> This is a bad experience, but it's basically "just" error handling. Under
> normal circumstances users won't be noticing this problem. So minusing for
> v1 but tracking for v1.x

Unlike bug 825223 where we're failing silently (which is less severe and I agree to track, not block) this one really produces nasty effects on the download experience.

Past precedent in triage states that the developer experience for apps does indeed block to provide at least some indication of why something failed, so along with Julien's comments in comment 42, I tend to think this still blocks.

Comment 44

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #43)
> (In reply to Jonas Sicking (:sicking) from comment #40)
> > This is a bad experience, but it's basically "just" error handling. Under
> > normal circumstances users won't be noticing this problem. So minusing for
> > v1 but tracking for v1.x
> 
> Unlike bug 825223 where we're failing silently (which is less severe and I
> agree to track, not block) this one really produces nasty effects on the
> download experience.
> 
> Past precedent in triage states that the developer experience for apps does
> indeed block to provide at least some indication of why something failed, so
> along with Julien's comments in comment 42, I tend to think this still
> blocks.

See https://bugzilla.mozilla.org/show_bug.cgi?id=816448 that discussed this in thorough detail of why error handling for general development experiences actually does block.

Comment 45

[Julien Wajsberg [:julienw]]

Even without talking about the error handling, my comment states that there is an actual blocking behaviour for updates...

Comment 46

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

> As I said in the comment with the patch, it also fixes a lot more serious
> issue that would make an update fails as soon as we save an ETag, so I'm
> renominating.

This is a blocker I agree.

(In reply to Jason Smith [:jsmith] from comment #43)
> Unlike bug 825223 where we're failing silently (which is less severe and I
> agree to track, not block) this one really produces nasty effects on the
> download experience.
> 
> Past precedent in triage states that the developer experience for apps does
> indeed block to provide at least some indication of why something failed, so
> along with Julien's comments in comment 42, I tend to think this still
> blocks.

We are simply at the point where we no longer can block on these types of issues though. If this was the only problem in this bug then I would still mark it blocking-minus, sad as it is. We always have developer docs and FAQs to help developers with bug hunting.

Not all developer-affecting bugs are equal. I agree that some of them are severe enough that they are blockers. But I don't think this one falls into that category. It only affects developers setting up app stores (or self-hosting packaged apps) and it only affects error reporting.

Comment 47

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

> > Please also put a call in onStartRequest which calls checks for an etag and
> > calls channel->Cancel() if there isn't an etag available.
> > 
> 
> Sorry but I don't understand the point of this. Do you want to abort if we
> have no saved ETag in the app object ? I'm afraid this might break our
> dogfood users because until recently we didn't have a saved ETag.

The idea is to call channel->getResponseHeader("Etag"). If that throws, then call cancel() on the channel so that we fail early and we fail in a consistent state.

Comment 48

[Julien Wajsberg [:julienw]]

(In reply to Jonas Sicking (:sicking) from comment #47)
> > > Please also put a call in onStartRequest which calls checks for an etag and
> > > calls channel->Cancel() if there isn't an etag available.
> > > 
> > 
> > Sorry but I don't understand the point of this. Do you want to abort if we
> > have no saved ETag in the app object ? I'm afraid this might break our
> > dogfood users because until recently we didn't have a saved ETag.
> 
> The idea is to call channel->getResponseHeader("Etag"). If that throws, then
> call cancel() on the channel so that we fail early and we fail in a
> consistent state.

Ok I'll try this !
Are you sure that we'll have received all the headers yet ?

Comment 49

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

Yes, that's the purpose of onStartRequest, it's called when we have all the metadata but before the actual request data starts coming in. In the case of HTTP, "metadata" means headers.

Comment 50

[Julien Wajsberg [:julienw]]

Thanks, this makes sense !

Then do we still need Bug 825218 ?

Comment 51

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

Yes. Fixing that will prevent the user from getting prompted about an update being available when in reality only the minimanifest has changed, but the actual application package has not.

Comment 52

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

However bug 825223 would be automatically fixed if we did that.

Comment 53

[Julien Wajsberg [:julienw]]

Attachment: patch v3

Jonas, I'd be more comfortable if you could review these changes.

Also, please go ahead and check this in if this is ok !

Thanks :)

Comment 54

[Jason Smith [:jsmith]]

I take it this patch is fixing a bug I just saw doing an update of a packaged app, which will spin forever right now and never finish updating:

12-30 00:49:44.452: E/GeckoConsole(109): [JavaScript Error: "NS_ERROR_XPC_NOT_ENOUGH_ARGS: Not enough arguments [nsIHttpChannel.setRequestHeader]" {file: "resource://gre/modules/Webapps.jsm" line: 1735}]

Comment 55

[Julien Wajsberg [:julienw]]

Jason> yes it is ;)

This is the part of the patch that I consider blocking.

Comment 56

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

Checked in. Unfortunately I had a copy-paste error when adding your username to the patch so it looks like it was written by someone else :(

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

In the future, it's great if you can use |hg export| when attaching patches for others to check in. That way your username and check-in comment gets attached to the patch.

Comment 57

[Julien Wajsberg [:julienw]]

jonas> fine, I wondered if hg had something like |git format-patch| :)

thanks !

Comment 58

[Julien Wajsberg [:julienw]]

To whoever will check in to other trees, this need to go in aurora and b2g18 as well.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

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

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d82cd241031e
https://hg.mozilla.org/releases/mozilla-b2g18/rev/7e9f9d3828ad