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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Stability, defect)

Description

[ben.song]

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

Comment 1

[ben.song]

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.

Comment 2

[ben.song]

Attachment: 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.

Comment 3

[Steven Yang [:styang]]

ni? chens as Yifan is on PTO this week.
Hi Chens,
You can refer to bug:1143526 for the tracking history. thanks.

Comment 4

[Sherman Chen [:chens] (inactive)]

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?

Comment 5

[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.

Comment 6

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

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

Comment 7

[Andrea Marchesini [:baku]]

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?

Comment 8

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(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.

Comment 9

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Hi Ben.

Could you help to confirm:

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

Thanks

Comment 10

[ben.song]

(In reply to Vance Chen [:vchen][[e-mail]] 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.

Comment 11

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

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

Comment 12

[Andrea Marchesini [:baku]]

I cannot reproduce it with the latest m-c. Which gecko + gaia version are you using?

Comment 13

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(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

Comment 14

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

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.

Comment 15

[ben.song]

(In reply to Vance Chen [:vchen][[e-mail]] 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.

Comment 16

[shawn ku [:sku]]

Rex, Please help check this case in parallel per offline talk.

Comment 17

[yifan [:yifan][:yliao]]

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 ) .

Comment 18

[Andrea Marchesini [:baku]]

Ok, I can reproduce it. I wrote half of the patch for this. Tomorrow we have a full fix.

Comment 19

[Andrea Marchesini [:baku]]

Attachment: fix.patch

This patch is for b2g v2.1.

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 21

[Andrea Marchesini [:baku]]

Attachment: m-i patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=579827c141bd

Comment 22

[Andrea Marchesini [:baku]]

Attachment: b2gv2.1 patch

Ready to test it? It works with my device. Give me a feedback.

Comment 23

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

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

Comment 24

[ben.song]

(In reply to Vance Chen [:vchen][[e-mail]] 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.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d3e2f504072

Comment 26

[Andrea Marchesini [:baku]]

before landing the patch in b2g v2.1 I want to see the result of the tests.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9d3e2f504072

Comment 28

[Daniel Holbert [:dholbert]]

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.

Comment 29

[Andrea Marchesini [:baku]]

Thanks Daniel, if you file a bug I can fix it quickly.

Comment 30

[Daniel Holbert [: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.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/49430a59175a

Comment 32

[Daniel Holbert [: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.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/49430a59175a

Comment 34

[ben.song]

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.

Comment 35

[ben.song]

ni Andrea, Vance for feedback.

Comment 36

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

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

Comment 37

[Andrea Marchesini [:baku]]

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.

Comment 38

[Steven Yang [:styang]]

(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.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7ce084b2a72b
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/33dca6238f59

Comment 40

[Vincent Liu[:vliu]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/33dca6238f59