Cannot add collection after homescreen kill

RESOLVED FIXED in Firefox 32, Firefox OS v2.0
(NeedInfo from)

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jlal, Assigned: amarchesini, NeedInfo)

Tracking

unspecified
mozilla33
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox32 fixed, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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?)
Component: Gaia::Homescreen → DOM: Device Interfaces
Product: Firefox OS → Core
Andrea - This feels like it could be a datastore issue. Could you take a look?
Flags: needinfo?(amarchesini)
(When the homescreen recovers we should do sync from datastore to get updated records - and those should appear)
This might be a dupe of bug 1023796, but I need a logcat on bug 1023796 to confirm.

Updated

4 years ago
Blocks: 989848

Updated

4 years ago
blocking-b2g: --- → 2.0?
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking+]
Actually before I triage this - how is the homescreen getting killed here? By LMK?
blocking-b2g: 2.0? → ---
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking+]
Flags: needinfo?(jlal)
Even if it is LMK, we might have a datastore bug on our hands here.
(Reporter)

Comment 6

4 years ago
I killed it via adb shell kid <homescreen pid> not sure how close it is to LMK these days.
Flags: needinfo?(jlal)
(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.
Created attachment 8439794 [details] [diff] [review]
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 :/
Attachment #8439794 - Flags: review?(amarchesini)

Updated

4 years ago
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking-]
Created attachment 8439914 [details] [diff] [review]
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.
Attachment #8439794 - Attachment is obsolete: true
Attachment #8439794 - Flags: review?(amarchesini)
Attachment #8439914 - Flags: review?(ehsan)
Flags: needinfo?(amarchesini)

Comment 10

4 years ago
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?
Attachment #8439914 - Flags: review?(ehsan) → review+
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.
Created attachment 8440079 [details] [diff] [review]
foobar.patch
Attachment #8439914 - Attachment is obsolete: true
Attachment #8440079 - Flags: review?(ehsan)

Updated

4 years ago
Attachment #8440079 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/8b3794b7e1dc
Assignee: nobody → amarchesini
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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?
Attachment #8440079 - Flags: approval-mozilla-aurora?
Bhavana - is there anything special I need to do to get this uplifted? Thanks!
Flags: needinfo?(bbajaj)
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?
Flags: needinfo?(bbajaj)
Flags: needinfo?(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.
Flags: needinfo?(kgrandon)
Attachment #8440079 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/9d21d16d60fa
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
status-firefox32: --- → fixed
status-firefox33: --- → fixed

Comment 21

4 years ago
Would you please provide the reproduce step for tester to verify this issue.Thanks for your help.
Flags: needinfo?(jlal)
You need to log in before you can comment on or make changes to this bug.