Closed Bug 1152169 Opened 5 years ago Closed 5 years ago

[FFOS7715 v2.1] after Power off test icons of homescreen disappear

Categories

(Firefox OS Graveyard :: Stability, defect, critical)

ARM
Gonk (Firefox OS)
defect
Not set
critical

Tracking

(blocking-b2g:2.1+, firefox38 wontfix, firefox39 wontfix, firefox40 fixed, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
2.2 S10 (17apr)
blocking-b2g 2.1+
Tracking Status
firefox38 --- wontfix
firefox39 --- wontfix
firefox40 --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: ben.song, Assigned: baku)

References

Details

Attachments

(3 files, 1 obsolete file)

Reproduce step:

1.Do poweroff test like bug 1115619 for 4~6 days.

2.Start-up the phone normally.

Ideal result:

Icons of homescreen display completely.

Actual result:

Icons of homescreen disappear

Reproduce rate: 4/4
blocking-b2g: --- → 2.1S?
Dear Vance,Yifan,

The problem in bug1143526 reproduce, after poweroff test icons of homescreen disappear, and the rate is 4/4.

After analyse the data module of problem device, i find the sqlite of bookmark still has some problems. I would upload the data information of normal device and problem device.

Thanks.
Flags: needinfo?(yliao)
Flags: needinfo?(vchen)
Attached file data module.zip
Dear Yifan, Vance,

The attachment contains data modules of normal device and problem device, please help me to analyse this problem.

Thanks.
See Also: → 1143526
ni? chens as Yifan is on PTO this week.
Hi Chens,
You can refer to bug:1143526 for the tracking history. thanks.
Flags: needinfo?(chens)
Ben, did you apply baku's patch from your side? 
I found that patch only lands on mozilla-central but not on mozilla-b2g34_v2_1s, could be the problem?
Flags: needinfo?(chens) → needinfo?(ben.song)
(In reply to Sherman Chen [:chens] from comment #4)
> Ben, did you apply baku's patch from your side? 
> I found that patch only lands on mozilla-central but not on
> mozilla-b2g34_v2_1s, could be the problem?

Dear Sherman,

I have applied the patch in local patch list, so in the log of this time we couldn't find logs like below:

03-20 21:10:59.230 E/GeckoConsole(  586): [JavaScript Error: "aEvent.target.result is undefined" {file: "resource://gre/modules/DataStoreCursorImpl.jsm" line: 173}]
03-20 21:10:59.240 E/GeckoConsole(  586): [JavaScript Error: "aEvent.target is undefined" {file: "resource://gre/modules/DataStoreCursorImpl.jsm" line: 76}]

Thanks.
Flags: needinfo?(ben.song) → needinfo?(chens)
Hi Baku, Still can reproduce the icon disappear issue with your patch in Bug#1143526, could you help to check datastore dump from partners?

Thanks
blocking-b2g: 2.1S? → 2.1S+
Flags: needinfo?(vchen) → needinfo?(amarchesini)
Can you help me to reproduce this issue? If I use mozilla_b2g34_v2_1s and I replace the profile directory + the other directories, should I be able to see the issue?
Flags: needinfo?(amarchesini) → needinfo?(vchen)
(In reply to Andrea Marchesini (:baku) from comment #7)
> Can you help me to reproduce this issue? If I use mozilla_b2g34_v2_1s and I
> replace the profile directory + the other directories, should I be able to
> see the issue?

If you have a Dolphin device then I think you should be able to reproduce this issue with v2.1s + replacing the corrupted directory data partner attached. But not sure this is going to help much since it just show the symptoms(that all icons in the homescreen are gone), but not how the directory is corrupted.

And I think if you only have a Flame device you can still reproduce this issue by replacing the corrupted directory data.
Flags: needinfo?(vchen)
Hi Ben.

Could you help to confirm:

If we put the corrupted data module to the Flame, could we still see the symptoms?

Thanks
Flags: needinfo?(ben.song)
(In reply to Vance Chen [:vchen][vchen@mozilla.com] from comment #9)
> Hi Ben.
> 
> Could you help to confirm:
> 
> If we put the corrupted data module to the Flame, could we still see the
> symptoms?
> 
> Thanks

Dear Vance,

After my try, I found while put 444928743bpopoakbmeawr.ktss_es.sqlite(/data/local/storage/persistent/chrome/idb) of corrupted data mudule to the Flame, icons of homescreen disappear.

Thanks.
Flags: needinfo?(ben.song) → needinfo?(vchen)
Hi Baku -

Per Comment#10, you can replace this issue on Flame by replacing the good sqlite db with the corrupted ones...But not sure if this helps since it won't show how the db is corrupted, but only shows the final symptoms(that all icons on the homescreen are gone)

Thanks
Flags: needinfo?(vchen) → needinfo?(amarchesini)
I cannot reproduce it with the latest m-c. Which gecko + gaia version are you using?
Flags: needinfo?(amarchesini) → needinfo?(vchen)
(In reply to Andrea Marchesini (:baku) from comment #12)
> I cannot reproduce it with the latest m-c. Which gecko + gaia version are
> you using?

So you cannot reproduce this issue after you replacing the sqlite data with the corrupted one partner provided? As I remember, our partner just use the 2.1 branch:

Gaia-Rev        db751d9c200dce41cd03a61665746d245739a175
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/28ffee0d5b0c
Build-ID        20150312001452
Version         34.0

Hi Song Ben, could you double check again the version of Flame you use to reproduce this issue?

Thanks
Flags: needinfo?(vchen) → needinfo?(ben.song)
Also here to provide more information about partner's "power-cut" test:

We use 10 devices to run the power-cut test. the test cycle is as following:

Prerequisite: Each device is connected with power supply machine such that we can cut the power of the device at any given time

Some initial value:

power_on_time = 8 sec
power_off_time = 3 sec
incremental_power_on_time

Test Cycle:

1. Power on device by pressing the power key and feel the device vibrates
2. "power_on_time" seconds after the device vibrates, cut the power of the device
3. "power_off_time" seconds after we cut the power of the device, reboot the device again
4. power_on_time += 2ms
5. Cut the power again when the device power on for "power_on_time" seconds
6. Repeat for 7 days.

And after the above test, all the 10 devices still can successfully power on, but all the icons on the homescreen are gone.
Flags: needinfo?(amarchesini)
(In reply to Vance Chen [:vchen][vchen@mozilla.com] from comment #13)
> (In reply to Andrea Marchesini (:baku) from comment #12)
> > I cannot reproduce it with the latest m-c. Which gecko + gaia version are
> > you using?
> 
> So you cannot reproduce this issue after you replacing the sqlite data with
> the corrupted one partner provided? As I remember, our partner just use the
> 2.1 branch:
> 
> Gaia-Rev        db751d9c200dce41cd03a61665746d245739a175
> Gecko-Rev      
> https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/28ffee0d5b0c
> Build-ID        20150312001452
> Version         34.0
> 
> Hi Song Ben, could you double check again the version of Flame you use to
> reproduce this issue?
> 
> Thanks

Dear Vance,

The version of our flame device is as below:

Gaia-Rev        a227b67d9ae647cc949ec1ef48ced00c80240025
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/a61c9c3dfba7
Build-ID        20150309161206
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  39
FW-Date         Thu Oct 16 18:19:14 CST 2014
Bootloader      L1TC00011880

If you couldn't reproduce it on Flame, please remove folder of ****+f+app+++verticalhome.gaiamobile.org before replacing the sqlite data with the corrupted one.

Thanks.
Flags: needinfo?(ben.song) → needinfo?(vchen)
Rex, Please help check this case in parallel per offline talk.
Flags: needinfo?(rhung)
Although the patch in 1143526 fixed the error dispatching bug, there is still no way for gaia to create a new DataStore by navigator.getDataStores API, as comments from 25 to 28 in bug 1143526 stated ( https://bugzilla.mozilla.org/show_bug.cgi?id=1143526#c25 ) .
Flags: needinfo?(yliao)
Ok, I can reproduce it. I wrote half of the patch for this. Tomorrow we have a full fix.
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attached patch fix.patch (obsolete) — Splinter Review
This patch is for b2g v2.1.
Attachment #8593015 - Flags: review?(bent.mozilla)
Comment on attachment 8593015 [details] [diff] [review]
fix.patch

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

::: dom/datastore/DataStoreService.cpp
@@ +578,5 @@
> +      }
> +
> +      // a Null JSContext is ok because OpenCursor ignores it if the range is
> +      // undefined.
> +      mRequest = store->OpenCursor(nullptr, JS::UndefinedHandleValue,

Yeah, I think you should use AutoSafeJSContext here too. Just because we don't need it today doesn't mean we won't in the future.

@@ +591,2 @@
>        if (NS_FAILED(rv)) {
> +        NS_WARNING("Failed to remove an EventListener.");

Nit: s/remove/add/

@@ +597,5 @@
>      }
>  
>      // The DB has just been created.
>  
> +    nsresult rv = CreateFirstRevision(aDb->Transaction());

Nit: You can just reuse ErrorResult here.

@@ +637,5 @@
> +  }
> +
> +  // nsIDOMEventListener
> +  NS_IMETHOD
> +  HandleEvent(nsIDOMEvent* aEvent)

You should swap mTxn and mRequest to stack nsRefPtrs here to ensure that you don't hold them alive past the end of this method. You'll need to then access them through the stack pointers.

@@ +648,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;
> +    }
> +
> +    if (!type.EqualsASCII("success")) {

This can all probably be DEBUG-only and asserted since you only added a listener for "success" events.

Might be good to assert that the target is 'this' also.

@@ +652,5 @@
> +    if (!type.EqualsASCII("success")) {
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    mRequest->RemoveEventListener(NS_LITERAL_STRING("success"), this, false);

You can't return without calling this, so I suggest you do this right after swapping refs to the stack refptrs.

@@ +655,5 @@
> +
> +    mRequest->RemoveEventListener(NS_LITERAL_STRING("success"), this, false);
> +
> +    // Note: this cx is only used for rooting and AddRevision, neither of which
> +    // actually care which compartment we're in.

This isn't right, GetResult will create and wrap an IDBCursor object... Maybe fix the comment?

@@ +666,5 @@
> +      return error.ErrorCode();
> +    }
> +
> +    // This means that the content is a IDBCursor, so the first revision already
> +    // exists.

Could we assert that?

@@ +686,5 @@
>  private:
>    ~FirstRevisionIdCallback() {}
>  
> +  nsRefPtr<IDBRequest> mRequest;
> +  nsRefPtr<IDBTransaction> mTxn;

Since these are CC'd objects I think you should explicitly null them out
Attachment #8593015 - Flags: review?(bent.mozilla) → review+
Attached patch b2gv2.1 patchSplinter Review
Ready to test it? It works with my device. Give me a feedback.
Hi Baku, thanks for your strong support on this critical issue!

Hi Song Ben -

Please help to test this patch and let us know the result

Thanks
Flags: needinfo?(vchen) → needinfo?(ben.song)
(In reply to Vance Chen [:vchen][vchen@mozilla.com] from comment #23)
> Hi Baku, thanks for your strong support on this critical issue!
> 
> Hi Song Ben -
> 
> Please help to test this patch and let us know the result
> 
> Thanks

Dear Vance,

We would land this patch and start poweroff test, if it success or fail i would give you feedback.

Thanks.
before landing the patch in b2g v2.1 I want to see the result of the tests.
https://hg.mozilla.org/mozilla-central/rev/9d3e2f504072
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S10 (17apr)
This introduced 2 clang 3.6 build warnings (treated as errors in enable-warnings-as-errors builds):
{
 dom/datastore/DataStoreService.cpp:561:3: error: 'Run' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
   Run(DataStoreDB* aDb, RunStatus aStatus)
   ^
 dom/datastore/DataStoreCallbacks.h:29:16: note: overridden virtual function is here
   virtual void Run(DataStoreDB* aDb, RunStatus aStatus) = 0;
                ^
}

{
 dom/datastore/DataStoreService.cpp:643:3: error: 'HandleEvent' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
   HandleEvent(nsIDOMEvent* aEvent)
   ^
 ../../dist/include/nsIDOMEventListener.h:31:14: note: overridden virtual function is here
   NS_IMETHOD HandleEvent(nsIDOMEvent *event) = 0;
}

(HandleEvent was added in this patch. Run() predated this patch, but wasn't flagged until now because there were no other functions that were labeled as 'override', so it wasn't inconsistent. This patch added NS_DECL_ISUPPORTS which includes some 'override' methods, though, so that made Run()'s lack-of-annotation inconsistent.)

I'll push a followup to fix this.
Thanks Daniel, if you file a bug I can fix it quickly.
Flags: needinfo?(dholbert)
Thanks! I've got a patch prepped already (just tagged as "bug 1152169 followup"); just waiting for tree to reopen so I can push it.
Flags: needinfo?(dholbert)
(In reply to Pulsebot from comment #31)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/49430a59175a

Pushed the followup ^, with blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2.
Dear Vance,

We have passed in recent power off test, I think attachment 8593229 [details] [diff] [review] works successfully. And we would do power test in next official version for further confirmation.

Thanks.
Flags: needinfo?(ben.song)
ni Andrea, Vance for feedback.
Flags: needinfo?(vchen)
Flags: needinfo?(amarchesini)
Hi Baku -

Partner confirms that your patch can solve this problem. Could you help to land it on 2.1?

ni Steven and Vincent as well for further landing on 2.1s
Flags: needinfo?(vliu)
Flags: needinfo?(vchen)
Flags: needinfo?(styang)
Flags: needinfo?(rhung)
Flags: needinfo?(chens)
Comment on attachment 8593229 [details] [diff] [review]
b2gv2.1 patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 916089
User impact if declined: if the a idb file is corrupted, the datastore cannot recover it.
Testing completed: on device.
Risk to taking this patch (and alternatives if risky): none. 
String or UUID changes made by this patch: none.
Flags: needinfo?(amarchesini)
Attachment #8593229 - Flags: approval-mozilla-b2g34?
(In reply to Andrea Marchesini (:baku) from comment #37)
> Comment on attachment 8593229 [details] [diff] [review]
> b2gv2.1 patch
> 
> NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to
> better understand the B2G approval process and landings.
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 916089
> User impact if declined: if the a idb file is corrupted, the datastore
> cannot recover it.
> Testing completed: on device.
> Risk to taking this patch (and alternatives if risky): none. 
> String or UUID changes made by this patch: none.

thanks baku. ni? RM for the approval.
Flags: needinfo?(styang) → needinfo?(bbajaj)
Flags: needinfo?(bbajaj)
Attachment #8593229 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
blocking-b2g: 2.1S+ → 2.1+
You need to log in before you can comment on or make changes to this bug.