935854 - Bad packaged app upload blocks all future upload attempts

Status: VERIFIED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

If you transfer a packaged app to the device, but it gets corrupted in transit, it's possible to get stuck in a state where the app can't ever be uploaded successfully anymore.

We currently skip moving the uploaded file (at /data/local/tmp/file-upload/package.zip) to the temporary application directory (at /data/local/tmp/b2g/APP_ID) if the directory already exists, but this is possible if a previous upload failed to complete correctly.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Always move uploaded package to installation dir

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

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 828498 [details] [diff] [review]
Always move uploaded package to installation dir

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

::: toolkit/devtools/server/actors/webapps.js
@@ +508,5 @@
> +        return { error: "badParameter",
> +                 message: "The uploaded file doesn't exist on device" };
> +      }
> +      appFile.moveTo(appDir, "application.zip");
> +    } else if (!aRequest.manifest && !aRequest.metadata) {

You should tweak this condition in order to not return this error 
if appDir && appDir.exists()

otherwise, you will just forbid app install via adb push.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Always move uploaded package to installation dir v2

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

(In reply to Alexandre Poirot (:ochameau) from comment #2)
> Comment on attachment 828498 [details] [diff] [review]
> Always move uploaded package to installation dir
> 
> Review of attachment 828498 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/server/actors/webapps.js
> @@ +508,5 @@
> > +        return { error: "badParameter",
> > +                 message: "The uploaded file doesn't exist on device" };
> > +      }
> > +      appFile.moveTo(appDir, "application.zip");
> > +    } else if (!aRequest.manifest && !aRequest.metadata) {
> 
> You should tweak this condition in order to not return this error 
> if appDir && appDir.exists()
> 
> otherwise, you will just forbid app install via adb push.

Thanks for catching this, I didn't even know it was something people used! :)

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://hg.mozilla.org/integration/fx-team/rev/e7d6e224ba57

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

[Petruta Horea [:phorea], Desktop QA]

Verified that using latest Firefox 28 beta 4 the zip file is also moved to the temporary application directory (at /data/local/tmp/b2g) while with an oldest Nightly it is only moved to file-upload folder.

Is this verification enough to mark this bug as Verified or we should also reproduce the case with an app that gets corrupted while updating? If yes, please let us know how to reproduce this case. Thank you!

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Does an "oldest Nightly" mean Firefox 30?  Or something before Firefox 28...?

It's a bit a little while since this happened, but I believe you could test more thoroughly by trying to transfer a large app (which would take a while), and disconnecting the device in the middle of the installation process the first time.  That would simulate the "corrupted" situation.

Then, if you are able to successfully install anyway a second time, that should be enough to verify this.

Comment 8

[Petruta Horea [:phorea], Desktop QA]

Thanks for the updates. I was using Firefox 28.0a1 (20131105030206).
On this version I couldn't reproduce the "corrupted situation" - the largest app I have is about 3MB and takes around 8 sec to install. 
I disconnected the device several times before actually let it be installed, but at the end it was installed and running.

The only thing I noticed is that the Update / Debug buttons remain grayed out in Apps tab after reconnecting the device. Switching to another app from list and returning will displayed the buttons correctly (happens on Fx 28 beta 6 too).

Should I try with a larger packaged app?

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Petruta Rasa [QA] [:petruta] from comment #8)
> Thanks for the updates. I was using Firefox 28.0a1 (20131105030206).
> On this version I couldn't reproduce the "corrupted situation" - the largest
> app I have is about 3MB and takes around 8 sec to install. 
> I disconnected the device several times before actually let it be installed,
> but at the end it was installed and running.
> 
> The only thing I noticed is that the Update / Debug buttons remain grayed
> out in Apps tab after reconnecting the device. Switching to another app from
> list and returning will displayed the buttons correctly (happens on Fx 28
> beta 6 too).
> 
> Should I try with a larger packaged app?

Just looked more closely at the patch again.  I had forgotten to say that this change is on device only, so the version of Firefox does not matter here.

If you wanted to reproduce the original problem, you would need a B2G build from before this patch landed.  Anyway, it's seems like this is working from what you've tested so far, so it seems fine to mark as verified to me.

Not sure if the debug buttons remaining grayed out after reconnection still applies to Nightly too, but if so it would be good to file a new bug for that issue (which is unrelated to this one).

Comment 10

[Petruta Horea [:phorea], Desktop QA]

Thanks Ryan!
I'll file a new bug for Update / Debug buttons.

I used an unagi device with B2G 1.2, platform version 26.0a2 and build identifier 20131014004003 and I was able to install the app after several disconnections.

I will mark this as verified based on previously results and Comment 9.