Closed Bug 980549 Opened 6 years ago Closed 6 years ago

Intermittently unable to unlock lock screen

Categories

(Core :: DOM: Device Interfaces, defect, critical)

30 Branch
ARM
Gonk (Firefox OS)
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla32
blocking-b2g 1.4+
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: bhargavg1, Assigned: bent.mozilla)

References

()

Details

(Whiteboard: [CR 627406])

Attachments

(2 files, 1 obsolete file)

In recent 1.4 builds see that the lock screen is enabled by default and unable to unlock.

Swiping towards the un-lock icon doesnt unlock the phone, this completely blocks testing.
blocking-b2g: --- → 1.4?
Blocks: 960372
Whiteboard: [CR 627406]
See the following in the settings.json file generated

"lockscreen.passcode-lock.timeout":0,
"lockscreen.passcode-lock.enabled":false,
"lockscreen.notifications-preview.enabled":true,
"lockscreen.enabled":false,
"lockscreen.locked":true,
"lockscreen.unlock-sound.enabled":false
(In reply to bhargavg1 from comment #0)
> In recent 1.4 builds see that the lock screen is enabled by default and
> unable to unlock.
> 
> Swiping towards the un-lock icon doesnt unlock the phone, this completely
> blocks testing.

Can you kindly provide the Build ID where you are seeing this?  Thanks.
(In reply to Marcia Knous [:marcia - use needinfo] from comment #2)
> (In reply to bhargavg1 from comment #0)
> > In recent 1.4 builds see that the lock screen is enabled by default and
> > unable to unlock.
> > 
> > Swiping towards the un-lock icon doesnt unlock the phone, this completely
> > blocks testing.
> 
> Can you kindly provide the Build ID where you are seeing this?  Thanks.

gaia  = dfae3744607257206e37483dc3f431108baf70fb
gecko = cbfe2d6c71daae6ac97b4681c432e950acf16ac6
Nobody is able to reproduce this on today's build - unlocking appears to work generally. However, there is an intermittent failure to unlock lockscreen known in bug 976196 - maybe that's what you are seeing?
(In reply to bhargavg1 from comment #3)
> (In reply to Marcia Knous [:marcia - use needinfo] from comment #2)
> > (In reply to bhargavg1 from comment #0)
> > > In recent 1.4 builds see that the lock screen is enabled by default and
> > > unable to unlock.
> > > 
> > > Swiping towards the un-lock icon doesnt unlock the phone, this completely
> > > blocks testing.
> > 
> > Can you kindly provide the Build ID where you are seeing this?  Thanks.
> 
> gaia  = dfae3744607257206e37483dc3f431108baf70fb
> gecko = cbfe2d6c71daae6ac97b4681c432e950acf16ac6

So that's about the time when we had a bricked build on trunk. There's loads of problems with that build. Can you retest on the latest changesets?
(In reply to Jason Smith [:jsmith] from comment #5)
> (In reply to bhargavg1 from comment #3)
> > (In reply to Marcia Knous [:marcia - use needinfo] from comment #2)
> > > (In reply to bhargavg1 from comment #0)
> > > > In recent 1.4 builds see that the lock screen is enabled by default and
> > > > unable to unlock.
> > > > 
> > > > Swiping towards the un-lock icon doesnt unlock the phone, this completely
> > > > blocks testing.
> > > 
> > > Can you kindly provide the Build ID where you are seeing this?  Thanks.
> > 
> > gaia  = dfae3744607257206e37483dc3f431108baf70fb
> > gecko = cbfe2d6c71daae6ac97b4681c432e950acf16ac6
> 
> So that's about the time when we had a bricked build on trunk. There's loads
> of problems with that build. Can you retest on the latest changesets?

the issue exists on the following builds as well,
gaia -  revision="71f78f7f68fcf5e23cc5965fee0dad45289c438b"
gecko - revision="d574219c44f75c9dfb007274e9de0479ef51020a"
(In reply to Jason Smith [:jsmith] from comment #4)
> Nobody is able to reproduce this on today's build - unlocking appears to
> work generally. However, there is an intermittent failure to unlock
> lockscreen known in bug 976196 - maybe that's what you are seeing?

We see this issue right when the device boots up for the first time after flashing the device. Sometimes after multiple reboots as well we couldnt get past the same...
(In reply to bhargavg1 from comment #6)
> (In reply to Jason Smith [:jsmith] from comment #5)
> > (In reply to bhargavg1 from comment #3)
> > > (In reply to Marcia Knous [:marcia - use needinfo] from comment #2)
> > > > (In reply to bhargavg1 from comment #0)
> > > > > In recent 1.4 builds see that the lock screen is enabled by default and
> > > > > unable to unlock.
> > > > > 
> > > > > Swiping towards the un-lock icon doesnt unlock the phone, this completely
> > > > > blocks testing.
> > > > 
> > > > Can you kindly provide the Build ID where you are seeing this?  Thanks.
> > > 
> > > gaia  = dfae3744607257206e37483dc3f431108baf70fb
> > > gecko = cbfe2d6c71daae6ac97b4681c432e950acf16ac6
> > 
> > So that's about the time when we had a bricked build on trunk. There's loads
> > of problems with that build. Can you retest on the latest changesets?
> 
> the issue exists on the following builds as well,
> gaia -  revision="71f78f7f68fcf5e23cc5965fee0dad45289c438b"
> gecko - revision="d574219c44f75c9dfb007274e9de0479ef51020a"

That's still within the timeframe of when we had a bricked build.
So this one's a regression!!

ni Tim for lockscreen
Flags: needinfo?(timdream)
This is not reproducing on any of the latest builds. Moving to works for me.
No longer blocks: 960372
Status: NEW → RESOLVED
blocking-b2g: 1.4? → ---
Closed: 6 years ago
Flags: needinfo?(timdream)
Resolution: --- → WORKSFORME
This issue is still reproducible in the latest m-c.

To clarify: the lock screen works fine when it's enabled by default. The problem is when we try to disable it by default at compile time by setting:

  "lockscreen.enabled":false

Perhaps disabling at compile time is just not supported anymore.
Okay - sounds like this is a customized build issue, not specific to QA's builds that we use on a daily basis.
Blocks: 960372
Status: RESOLVED → REOPENED
blocking-b2g: --- → 1.4+
Resolution: WORKSFORME → ---
Tim

Please review for lockscreen
Flags: needinfo?(timdream)
:snowmantw, is there other 1.4+ bugs in your queue? If not please have this one cut in line now. Thanks.
Assignee: nobody → gweng
Flags: needinfo?(timdream) → needinfo?(gweng)
Try it later. Don't know that happened, I didn't check-in any code in the recent master and just flashed yesterday's build from PVT.
Flags: needinfo?(gweng)
Summary: Unable to unlock lock screen → Unable to unlock lock screen if lock screen is disabled on build time
Summary: Unable to unlock lock screen if lock screen is disabled on build time → Unable to unlock lock screen if lock screen is disabled by default at build time
(In reply to Diego Wilson [:diego] from comment #12)
> This issue is still reproducible in the latest m-c.
> 
> To clarify: the lock screen works fine when it's enabled by default. The
> problem is when we try to disable it by default at compile time by setting:
> 
>   "lockscreen.enabled":false
> 
> Perhaps disabling at compile time is just not supported anymore.

I don't know. How did you set the flag off? Directly change the value written in the JavaScript file? Set the flag like 'NO_LOCK_SCREEN=1 make reset-gaia'? The later works for me: the lockscreen would not disappear after the booting, and it still works even after I set the lockscreen enabled via the Settings' entry.
The flag still works on:

ZTE Open
Version: v1.40
Platform: 30.0a1
Build: 20140311040203
Git commit info: 2014-03-12 03:23:13 c9285967
Flags: needinfo?(dwilson)
Oh, sorry, one typo:

" the lockscreen would not disappear" ->
" the lockscreen would not display"
Re-description STR:

1. flash my ZTE Open from PVT build tool, choose the newest master and flash Gecko + Gaia
2. checkout mozilla Gaia and reset to the newest master
3. NO_LOCK_SCREEN=1 make reset-gaia
4. the device would *not* show the lockscreen after FTU
5. enable the lockscreen in Settings
6. turn the screen off to lock the screen
7. turn the screen on and you will see the lockscreen
8. unlock, it works fine as before.
> Set the flag like 'NO_LOCK_SCREEN=1 make reset-gaia'?

This works, thanks!
Flags: needinfo?(dwilson)
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → WORKSFORME
We still see this issue sometimes and doesnt go away till multiple re-boots (somtimes close to 10 times) are there any logs that we can enable to see what's happening
Greg,

Looks like the issue was never fixed, just masked.

I think there's a race condition that somehow messes up the settings database. Any idea of what's happening here?

E/GeckoConsole(  203): [JavaScript Error: "InvalidStateError: A mutation operation was attempted on a database that did not allow mutations." {file: "jar:file:///system/b2g/omni.ja!/components/SettingsManager.js" line: 45}]
E/GeckoConsole(  203): [JavaScript Error: "NS_ERROR_UNEXPECTED: " {file: "app://system.gaiamobile.org/js/lockscreen_connection_info_manager.js" line: 99}]
E/GeckoConsole(  203): [JavaScript Error: "InvalidStateError: A mutation operation was attempted on a database that did not allow mutations." {file: "jar:file:///system/b2g/omni.ja!/components/SettingsManager.js" line: 45}]
E/GeckoConsole(  203): [JavaScript Error: "NS_ERROR_UNEXPECTED: " {file: "app://system.gaiamobile.org/js/lockscreen.js" line: 984}]
Status: RESOLVED → REOPENED
Flags: needinfo?(gweng)
Resolution: WORKSFORME → ---
Could the lock screen be trying to set a setting before its database is ready?
What did you mean? Does it mean that I need to mock a setting value before the database is ready, and then switch to detect the database after it's ready? I can try this solution, but I don't know how to test if the race condition would exactly disappear.
Flags: needinfo?(gweng)
Arthur suggested me to delay the writing operation after listened to the |load| event, because the settings DB may be not ready at the time the LockScreen got loaded.

This would only be a workaround, because the settings DB would be not ready at that time is a known issue. This bug would be completely solved after I refactor the LockScreen to initialize it after the window got completely loaded.
Ben?
Flags: needinfo?(bent.mozilla)
Would be able to get a patch for this blocker this week? QC would like to call our FC done and this is preventing the milestone from being reached.
Flags: needinfo?(gweng)
I'm not familiar with settings DB...so I even have no idea how to fix this properly yet.
One major question is how can LockScreen know when the settings DB now is ready.

I'll try to ask some colleagues in Taipei Office. However if someone has answer or references please tell me.
Flags: needinfo?(gweng)
Evelyn and Arthur have given me some suggestions:

1. Because the code it reported is within another |get| operation, it seems irrelevant to the code itself, or the first |get| operation should be failed first.

2. We have |getSettingsLock| before we manipulate the DB. If the lock works fine, this should not happen.

3. DB should be ready before any app runs. So we wonder if it's really the root cause.

4. Maybe someone else is writing/reading the field at the same time. But this may show the lock mentioned in 2. is not work as we thought.

5. NI Gregor to see if he has any opinion.

And I still can't reproduce it on my device...
Flags: needinfo?(anygregor)
This code (SettingsManager.js line 45) only runs after we successfully opened the settingsDB. It seems like we are failing to get the object store.
Ben, any idea? http://mxr.mozilla.org/mozilla-central/source/dom/settings/SettingsManager.js#45
Flags: needinfo?(anygregor)
The error means that the transaction has already committed and is being used afterwards.

Has anything changed in the settings code recently? Nothing in the IDB code has that I can think of... I guess it's possible that there's some error case in the settings code that we're not handling properly?

Also, why is a non-default build problem like this blocking? Or am I misunderstanding the issue?
Flags: needinfo?(bent.mozilla)
> Also, why is a non-default build problem like this blocking? Or am I
> misunderstanding the issue?

This blocks automation based testing.
What's the ETA of this fix? Our stability testing is blocked due to this issue.
Preeti, were you able to find an ETA?
Flags: needinfo?(praghunath)
Since I have no clue about how to solve or workaround these settings db issue, I shall deassign this bug.
Assignee: gweng → nobody
Flags: needinfo?(praghunath)
According to comment 30 comment 31 and comment 32 this is not a Gaia issue. Need some Gecko investigations.
Component: Gaia::System::Lockscreen → General
Component: General → DOM: Device Interfaces
Product: Firefox OS → Core
Version: unspecified → 30 Branch
Andrea is finishing up bug 974893 today but will investigate here as soon as that's done.
Assignee: nobody → amarchesini
(In reply to Andrew Overholt [:overholt] from comment #38)
> Andrea is finishing up bug 974893 today but will investigate here as soon as
> that's done.
 
Thanks Andrea! This looks like a bug in the SettingsManager.js. I will steal if I can reproduce.
Duplicate of this bug: 976196
Duplicate of this bug: 987391
I am renaming this due to the fact that it has been confirmed that we can see this also by disabling the lock screen from settings, and not the build script.

Reproduction rate is quite rare, I'm seeing it maybe once every 50 reboots or so.
Summary: Unable to unlock lock screen if lock screen is disabled by default at build time → Intermittently unable to unlock lock screen
(In reply to Kevin Grandon :kgrandon from comment #42)
> Reproduction rate is quite rare, I'm seeing it maybe once every 50 reboots
> or so.

I was seeing it daily. Here's my STR if it helps:

1. disable lock screen
2. backup user data
3. flash phone with new build
4. restore user data

Scripts I used for backup and restore user data are extremely simple: https://github.com/autonome/fxos-scripts
I'm working on this bug but I have to admin that I have hard time to reproduce it.
(In reply to Andrea Marchesini (:baku) from comment #44)
> I'm working on this bug but I have to admin that I have hard time to
> reproduce it.

Have we tried with the STR in comment #43?
Yes. I have the latest m-c, + the last gaia release.

1. disable lock screen from settings
2. backup user data
3. flash phone with the new build
4. restore user data
5. the device boots without lock screen
6. settings to enable the lock screen again
7. lock the device, and the lock screen is shown
8. unlock the device. it works.

what am I doing wrong?
I built yesterday, tried this hundreds of times on two different devices, and was no longer able to reproduce.

Given the major changes Yuren has been landing in lockscreen in the last week perhaps the problem is fixed, or as Bent suggested on IRC, some timing has changed, making the problem harder to reproduce.
Dietrich, which is the bug that tracked Yuren's lockscreen bug?  QC may want to take that for their testing.
Flags: needinfo?(dietrich)
Sorry, this is Greg's work, not Yuren. Bug 960915 landed about a week ago and bug 937442 stuck a few days ago.
Flags: needinfo?(dietrich)
I have looked at this code a bunch in the last few days and don't see any immediate problems. Also, I cannot reproduce after tons of tries.

Inder, is this still reproducible on your end? If it is then my next idea is to simplify the settings code a little bit and hope that things are magically fixed without regressing anything...
Flags: needinfo?(ikumar)
:bent -- yes this is still an issue on our end.
FYI, for FC we have locked our gecko/gaia on v1.4 to following SHA1s to avoid issues due to code churn on v1.4.
Gecko: d02b9250ef7fedafc6c709dfcc899844b8624ab6
Gaia: 4c3b2f57f4229c5f36f0d8fd399e65f4db88f104

If we have a patch/fixes that could possibly fix this issue then we can bring it in our tree to try out.
Flags: needinfo?(ikumar)
Hi Ben,

I'm able to reproduce this on the tip of v1.4 as of a day ago. It happened on the second try on my QRD 8x26.

Are you testing on m-c or v1.4?
Flags: needinfo?(bent.mozilla)
BTW I still see the exact same settings error message as in comment 23 when this happens.
Attached patch Patch, v1 (obsolete) — Splinter Review
Ok, I think I found it.

Inder, can you give this a shot?
Assignee: amarchesini → bent.mozilla
Status: REOPENED → ASSIGNED
Attachment #8404991 - Flags: review?(anygregor)
Flags: needinfo?(bent.mozilla)
Sure, we will give it a shot and get back here.
Comment on attachment 8404991 [details] [diff] [review]
Patch, v1

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

We should also fix it in SettingsService.js
Comment on attachment 8404991 [details] [diff] [review]
Patch, v1

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

We should also fix it in SettingsService.js
Attachment #8404991 - Flags: review?(anygregor) → review+
(In reply to Gregor Wagner [:gwagner] from comment #57)
> We should also fix it in SettingsService.js

It looks unaffected since it only accepts one key/value pair for set calls.
Diego was able to consistently reproduce it, adding ni for him.
Flags: needinfo?(dwilson)
Hi Ben,

I just tested your patch. Didn't work :( I see the same setting error in logcat too.
Flags: needinfo?(dwilson) → needinfo?(bent.mozilla)
Hm, are you sure you applied/rebuilt correctly? I was finally able to reproduce this on 1.4 but with this patch I cannot reproduce any longer.
Flags: needinfo?(bent.mozilla)
Attached patch Moar simple, v1Splinter Review
Diego, can you try this one too? It's more invasive (and unnecessary I think... the previous patch still seems to work fine for me), but maybe it makes a difference on your end?
Attachment #8405544 - Flags: feedback?(dwilson)
Comment on attachment 8405544 [details] [diff] [review]
Moar simple, v1

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

20 flash/reboots on my device and no probs. Works for me!
Attachment #8405544 - Flags: feedback?(dwilson) → feedback+
Well, that's good I guess. I'm still wondering why the first patch didn't fix it for you though...
Attachment #8405544 - Attachment description: Further simplification → Moar simple, v1
Attachment #8405544 - Flags: review?(anygregor)
Attachment #8404991 - Attachment is obsolete: true
Comment on attachment 8405544 [details] [diff] [review]
Moar simple, v1

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

Thanks!
Attachment #8405544 - Flags: review?(anygregor) → review+
Is attachment 8405544 [details] [diff] [review] landable?
Flags: needinfo?(bent.mozilla)
Ben

What would our next steps be to get a fix for this issue?

This is blocking QC testing and would like to provide a path forward if possible.

Please educate.
Flags: needinfo?(bent.mozilla)
Blocks: 984663
No longer blocks: 960372
Found the last piece today with gwagner.

https://hg.mozilla.org/integration/b2g-inbound/rev/a24917a68821
Flags: needinfo?(bent.mozilla)
Attached file Settings order changes
Ok, Gregor and I have been debugging this more or less constantly, and we've narrowed things down a bit.

First, the tests pass occasionally with this patch, and fail occasionally without it. So we're dealing with a real race that is not caused by this patch.

It looks like the tests in question fail because 'animationend' events don't fire at the correct time. This was pretty surprising. Gregor noticed that we can tell when the process launches whether or not it is going to fail because the failure case never actually displays the test app. It only shows the homescreen background and a slightly darker dock band, though no icons are ever shown. In the success case we don't see the homescreen but instead the test app. So it appears that the race breaks the test by changing something in the startup path.

I've attached a diff of the settings logging between the success case and the failure case. Both logs were taken with this patch applied! The only difference is I made a small change to indexedDB to slow down callback delivery on the main thread. I believe that one of these ordering changes is causing the test app to never be displayed properly, and thereafter the tests fail because the animation events are not delivered as expected.

I need some help to figure out what these settings changes mean.
We have some race condition when we start the test-agent and getting the ftu-skip event to display the home screen.

the breaking order is:

1) call launch for the test-agent and we call DISPLAY: http://test-agent.gaiamobile.org:8080
2) receive the ftu-skip event and call display.

It works if we get 2) before 1)
We need to fix the race conditions here. The fix in bug 998255 should resolve some of them, but there are still additional race conditions. I'm going to place a dependency on bug 998255 as this should help some of the settings ordering weirdness that we are seeing in the attachment here: https://bug980549.bugzilla.mozilla.org/attachment.cgi?id=8409469
Depends on: 998255
Depends on: 999574
Depends on: 999676
No longer depends on: 998255
Andrew,

Can we please open a bug to address the race condition issue?
Flags: needinfo?(overholt)
(In reply to Preeti Raghunath(:Preeti) from comment #76)
> Andrew,
> 
> Can we please open a bug to address the race condition issue?

Is there something specific you need here? I think we are resolving them in the dependent bugs. Once they are reviewed and landed we should be able to land this one.
Flags: needinfo?(overholt)
(In reply to Kevin Grandon :kgrandon from comment #77)
> (In reply to Preeti Raghunath(:Preeti) from comment #76)
> > Andrew,
> > 
> > Can we please open a bug to address the race condition issue?
> 
> Is there something specific you need here? I think we are resolving them in
> the dependent bugs. Once they are reviewed and landed we should be able to
> land this one.

So, based on comments 63 - 65 looks like the patch resolves the issue but not the race condition. If my understanding is right, then I was suggesting landing the patch and resolving the race condition in a separate bug. That way, QC has something to get started with. But you are the expert here and will leave the decision to you.
I agree with Preeti. Especially considering that the race condition seems to be in the tests. So looks like attachment 8405544 [details] [diff] [review] is not at fault here. What do you think bent?
Flags: needinfo?(bent.mozilla)
We cannot land this patch as-is because it causes our test suite to fail very frequently (nearly permanently). We would have to disable tests to get it to stick, and that approach has many associated risks.
Flags: needinfo?(bent.mozilla)
I see.

Will the attachment 8405544 [details] [diff] [review] be able to land as-is when bug 999574 is addressed? Should be focus on moving bug 999574 forward?
Flags: needinfo?(bhargavg1)
Flags: needinfo?(bhargavg1) → needinfo?(bent.mozilla)
(In reply to Diego Wilson [:diego] from comment #81)
> I see.
> 
> Will the attachment 8405544 [details] [diff] [review] be able to land as-is
> when bug 999574 is addressed? Should be focus on moving bug 999574 forward?

I am currently working on this. I have a patch which works, but there are some design questions about it, so I need to rewrite it and test. I hope to have something finished this week.
Flags: needinfo?(bent.mozilla)
No longer depends on: 999676
(In reply to Kevin Grandon :kgrandon from comment #82)
> (In reply to Diego Wilson [:diego] from comment #81)
> > I see.
> > 
> > Will the attachment 8405544 [details] [diff] [review] be able to land as-is
> > when bug 999574 is addressed? Should be focus on moving bug 999574 forward?
> 
> I am currently working on this. I have a patch which works, but there are
> some design questions about it, so I need to rewrite it and test. I hope to
> have something finished this week.

Kevin

Did you have a patch for the same? Are there any issues that we need help from partners.
(In reply to Preeti Raghunath(:Preeti) from comment #83)
> (In reply to Kevin Grandon :kgrandon from comment #82)
> Kevin
> 
> Did you have a patch for the same? Are there any issues that we need help
> from partners.

Yes, there is a patch in the dependent bug that is undergoing review. Nothing we need from partners right now.
(In reply to Kevin Grandon :kgrandon from comment #84)
> (In reply to Preeti Raghunath(:Preeti) from comment #83)
> > (In reply to Kevin Grandon :kgrandon from comment #82)
> > Kevin
> > 
> > Did you have a patch for the same? Are there any issues that we need help
> > from partners.
> 
> Yes, there is a patch in the dependent bug that is undergoing review.
> Nothing we need from partners right now.

Thanks much Kevin.

Does that mean if bug 999574 lands on 1.4 this bug can be closed?
https://hg.mozilla.org/mozilla-central/rev/15795f57fb94
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Flags: in-moztrap?
Found Test Case: https://moztrap.mozilla.org/manage/case/2533/

STR needs to added to verify this bug to the existing test case.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Flags: in-moztrap? → in-moztrap?(ychung)
Test case added to moztrap:

https://moztrap.mozilla.org/manage/case/14382/
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(ychung)
Flags: in-moztrap+
You need to log in before you can comment on or make changes to this bug.