1024618 - Cannot add collection after homescreen kill

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[James Lal [:lightsofapollo]]

The collections app will show up but after saving the collection nothing appears on the homescreen:

I see this in logcat

E/GeckoConsole( 1387): [JavaScript Error: "NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMessageSender.sendAsyncMessage]" {file: "resource://gre/modules/DataStoreChangeNotifier.jsm" line: 61}]

Which is likely the error (making this a DataStore bug?)

Comment 1

[Kevin Grandon :kgrandon]

Andrea - This feels like it could be a datastore issue. Could you take a look?

Comment 2

[Kevin Grandon :kgrandon]

(When the homescreen recovers we should do sync from datastore to get updated records - and those should appear)

Comment 3

[Jason Smith [:jsmith]]

This might be a dupe of bug 1023796, but I need a logcat on bug 1023796 to confirm.

Comment 4

[Jason Smith [:jsmith]]

Actually before I triage this - how is the homescreen getting killed here? By LMK?

Comment 5

[Kevin Grandon :kgrandon]

Even if it is LMK, we might have a datastore bug on our hands here.

Comment 6

[James Lal [:lightsofapollo]]

I killed it via adb shell kid <homescreen pid> not sure how close it is to LMK these days.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kevin Grandon :kgrandon from comment #5)
> Even if it is LMK, we might have a datastore bug on our hands here.

We found a datastore bug with the other homescreen proto that shows exactly the same error than the one you are seeing. Will open a bug with our local patch for it.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: broadcast.message.dont.throw.patch

Andrea, I tried to do a cleaner with a method that check if the message manager is still alive but didn't find the right method for that :/

Comment 9

[Andrea Marchesini [:baku]]

Attachment: foobar.patch

Sorry Vivien but your patch is not good. The real problem was that, if an app is registered to many dataStore, when it quits/is killed, DataStoreChangeNotifier.jsm doesn't remove all the instances of this app in its hashtable.

Comment 10

[(no longer active)]

Comment on attachment 8439914 [details] [diff] [review]
foobar.patch

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

Oops.  Can you add a test for this please?

Comment 11

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25b0cc03110c

I tried to create a test. It's not so easy to reproduce with a mochitest. I'll try again.
I land this patch because I spoke with ehsan on IRC and he agreed to land it and to write the mochitest in a separated patch.

Comment 12

[Ed Morley [:emorley]]

Backed out for failures in test_changes.html:
https://tbpl.mozilla.org/php/getParsedLog.php?id=41685326&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=41685116&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/264e4791ad72

Comment 13

[Andrea Marchesini [:baku]]

Attachment: foobar.patch

Comment 14

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b3794b7e1dc

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

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8b3794b7e1dc

Comment 16

[Kevin Grandon :kgrandon]

Comment on attachment 8440079 [details] [diff] [review]
foobar.patch

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

This is required for the vertical homescreen. Is the process the same here for uplifting as it is for gaia patches?

Comment 17

[Kevin Grandon :kgrandon]

Bhavana - is there anything special I need to do to get this uplifted? Thanks!

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

Kevin - For uplift approval you either need to have your bug marked as blocking-b2g (use the flag to nom) or request uplift approval as you've done. Please fill out the uplift questionnaire, which should autopopulate when you set approval-mozilla-aurora?

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch:

In this case, I see that the patch bounced, but had a successful try push and has now relanded. Can you comment on any additional testing that you have done?

What are the risks to taking this patch?

In a quick glance at the patch I do see hardcoded strings but I think they are all in debug statements. Did I miss any that require l10n?

Comment 19

[Kevin Grandon :kgrandon]

(In reply to Lawrence Mandel [:lmandel] from comment #18)
> User impact if declined: The bug. Users won't be able to add collections to the vertical homescreen.
> Testing completed (on m-c, etc.): Lots of manual testing and verification for now. A CI test is coming as a follow-up.
> Risk to taking this patch (and alternatives if risky): See below.
> String or IDL/UUID changes made by this patch: See below.
> 
> In this case, I see that the patch bounced, but had a successful try push
> and has now relanded. Can you comment on any additional testing that you
> have done?

It's been baking on master for a while now, and we've done lots of manual testing to verify that this is fine.

> What are the risks to taking this patch?

It's already broken, so not much, but if there were an area of regression it would be in datastore.

> In a quick glance at the patch I do see hardcoded strings but I think they
> are all in debug statements. Did I miss any that require l10n?

No l10n changes necessary.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9d21d16d60fa

Comment 21

[Elie]

Would you please provide the reproduce step for tester to verify this issue.Thanks for your help.