827047 - A signed packaged app update through Marketplace never triggers Gaia app update notification (see attached testcase)

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Jason Smith [:jsmith]]

Attachment: Logcat

Followup to bug 823150 after doing a bug verification test. See https://bugzilla.mozilla.org/show_bug.cgi?id=823150#c0 for STR & expected vs. actual results.

Looks like we still do not get the update notification prompt to appear upon trying to do a manual sync. When I check the logcat, it looks like we do know there is an update available:

01-05 13:34:17.870: I/Gecko(109): UpdatePrompt: appsUpdated: 1 apps to update

But no update prompt is showing up in Gaia. The gecko piece of bug 823150 *might* be working, but the Gaia piece definitely isn't.

Comment 1

[Jason Smith [:jsmith]]

Build: B2G 18 1/5/2013
Device: Unagi

Comment 2

[Etienne Segonzac (:etienne)]

How long did you wait after the manual check? The notification is still shown after a 30sec "grouping timeout" by default.

Comment 3

[Etienne Segonzac (:etienne)]

Yep, getting the notification exactly 30sec after the manual check here.
This looks like a duplicate of the (minused :/) bug 814055.

Comment 4

[Jason Smith [:jsmith]]

(In reply to Etienne Segonzac (:etienne) from comment #3)
> Yep, getting the notification exactly 30sec after the manual check here.
> This looks like a duplicate of the (minused :/) bug 814055.

I'll try again, but when I tried this, I didn't get a notification at all for at least 10 minutes.

Comment 5

[Jason Smith [:jsmith]]

Marking qawanted for a retest.

Comment 6

[Jason Smith [:jsmith]]

Btw Etienne - How did you test this? I've been testing the packaged app update mechanism through marketplace dev with signed packaged apps. Are you testing this differently possibly than the way I'm testing it?

Comment 7

[Jason Smith [:jsmith]]

re-tested with the existing app I already have installed that's v1.0 - no update found, still get same issue:

01-06 10:39:26.759: I/Gecko(507): UpdatePrompt: appsUpdated: 1 apps to update

And no notification. I've waited 10 minutes and I'm not seeing an update available.

I think we're going to need to get understanding of the difference on how I'm testing this through marketplace vs. how you are testing it. It's possible that this only reproduces through marketplace because of something marketplace is doing to serve the update.

Comment 8

[Jason Smith [:jsmith]]

Well 4 minutes so far, actually. But I'll drop a comment if I see something pop up.

Adding Rob to see if he explain what marketplace is doing when I decide to push a packaged app public from 1.0 --> 1.1. 

Test app being used - https://marketplace-dev.allizom.org/app/privileged-app-test-6/?src=mkt-search

Comment 9

[Jason Smith [:jsmith]]

No dice after 10 minutes :(.

Comment 10

[Etienne Segonzac (:etienne)]

(In reply to Jason Smith [:jsmith] from comment #6)
> Btw Etienne - How did you test this? I've been testing the packaged app
> update mechanism through marketplace dev with signed packaged apps. Are you
> testing this differently possibly than the way I'm testing it?

Yep I'm testing with a classic packaged app on my own server (note that this part of gaia is not making any difference between certified package apps and classic packaged apps).

Getting some marketplace credentials might be useful here, who should I ask?

Comment 11

[Jason Smith [:jsmith]]

(In reply to Etienne Segonzac (:etienne) from comment #10)
> (In reply to Jason Smith [:jsmith] from comment #6)
> > Btw Etienne - How did you test this? I've been testing the packaged app
> > update mechanism through marketplace dev with signed packaged apps. Are you
> > testing this differently possibly than the way I'm testing it?
> 
> Yep I'm testing with a classic packaged app on my own server (note that this
> part of gaia is not making any difference between certified package apps and
> classic packaged apps).
> 
> Getting some marketplace credentials might be useful here, who should I ask?

Rob Hudson should be able to get you access to marketplace dev.

Let's talk in the morning - I have access to marketplace dev, so let's see if we can debug what's going on here.

Comment 12

[Dietrich Ayala (:dietrich)]

Blocking- due to requiring manual update check. If updates never occur on the typical use-case (schedule update checks), then re-nom.

Comment 13

[Jason Smith [:jsmith]]

(In reply to Dietrich Ayala (:dietrich) from comment #12)
> Blocking- due to requiring manual update check. If updates never occur on
> the typical use-case (schedule update checks), then re-nom.

They don't ever happen. Ever. I don't think this has anything to do with whether it's manual or automatic - see the comment 0 for why. It's finding the update, but the user can't complete it.

Renoming to block.

Comment 14

[Jason Smith [:jsmith]]

On a different random note - I'd actually don't think the rationale to not block on manual sync makes sense anyways. That's directly part of v1 requirements, how people in the work week will be generally testing updates, etc. We need this fixed - I don't want our 3rd party app update team getting blocked by this.

Comment 15

[Dietrich Ayala (:dietrich)]

Thanks for clarifying -> +

Comment 16

[Jason Smith [:jsmith]]

Confirmed this with Etienne - yeah this shouldn't have anything to do with whether it's manual or automatic.

Comment 17

[Jason Smith [:jsmith]]

Apparently this doesn't fail with all possible packaged app updates - it fails with the test case I had in the original bug this was tied to.

When I tried this with a different test app combo, I got the update notification.

Comment 18

[Jason Smith [:jsmith]]

Attachment: Privileged App v1.0

Comment 19

[Jason Smith [:jsmith]]

Attachment: Privileged App v1.1

Comment 20

[Jason Smith [:jsmith]]

I'm starting to wonder if this is a gecko issue. Putting needsinfo on Fabrice to see what he thinks.

Comment 21

[Rob Hudson [:robhudson]]

(In reply to Jason Smith [:jsmith] from comment #8)
> Adding Rob to see if he explain what marketplace is doing when I decide to
> push a packaged app public from 1.0 --> 1.1. 

When a reviewer pushes to public behind the scenes we:
* Sign the package with the public certs which also copies them on the filesystem to the correct path to be available publicly.
* Update the mini-manifest so it contains information about the new version and points to the correct download path.
* Updates the search index so the details about the package are updated while searching.

I've verified previously that pushing to public updates the manifest and the file almost immediately after pushing it by using curl/wget to fetch the manifest and zip file.

Comment 22

[Julien Wajsberg [:julienw]]

Rob> have you checked that the ETag changed too ?

Comment 23

[[:fabrice] Fabrice Desré]

(In reply to Jason Smith [:jsmith] from comment #20)
> I'm starting to wonder if this is a gecko issue. Putting needsinfo on
> Fabrice to see what he thinks.

If you see the "01-06 10:39:26.759: I/Gecko(507): UpdatePrompt: appsUpdated: 1 apps to update" log, this means that the checkForUpdate() call effectively found an update. What we could do is check that gaia effectively received the app manifest url that needs to be updated.

Comment 24

[Etienne Segonzac (:etienne)]

(In reply to Fabrice Desré [:fabrice] from comment #23)
> (In reply to Jason Smith [:jsmith] from comment #20)
> > I'm starting to wonder if this is a gecko issue. Putting needsinfo on
> > Fabrice to see what he thinks.
> 
> If you see the "01-06 10:39:26.759: I/Gecko(507): UpdatePrompt: appsUpdated:
> 1 apps to update" log, this means that the checkForUpdate() call effectively
> found an update. What we could do is check that gaia effectively received
> the app manifest url that needs to be updated.

Gaia only use ondownloadavailable callback BTW we're not looking a the mozchromeevent details.

Comment 25

[Dale Harvey (:daleharvey)]

Attachment: Send checkForUpdate messages to all registered listeners

Comment 26

[Dale Harvey (:daleharvey)]

Attachment: Send successful checkForUpdate messages to all registered listeners

Updated to include other instances of application updates

Comment 27

[[:fabrice] Fabrice Desré]

Comment on attachment 699211 [details] [diff] [review]
Send successful checkForUpdate messages to all registered listeners

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

::: dom/apps/src/Webapps.jsm
@@ +831,5 @@
>    // Webapps:RemoveApp
>    // Webapps:Install:Return:OK
>    // Webapps:Uninstall:Return:OK
>    // Webapps:OfflineCache
> +  // Webapps:checkForUpdate:Return:OK

Nit: can you also add Webapps:PackageEvent to the list?

Comment 28

[Dale Harvey (:daleharvey)]

Attachment: Send successful checkForUpdate messages to all registered listeners

Added PackageEvent to commented broadcast messages

Comment 29

[Jason Smith [:jsmith]]

Ed - Can you help land this on inbound and b2g18?

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97fa8edaa376

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/a07224438b0c

Comment 32

[Jason Smith [:jsmith]]

inbound + b2g18 = closing.

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/97fa8edaa376

Comment 34

[Jason Smith [:jsmith]]

Looks good - tested this on prod and I got the update to fire.