1106324 - add missing call to removeLock in SettingsRequestManager.finalizeSets

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Call removeLock in case where the call was missing

While I was debugging bug 1105511, khuey noticed that one case was
missing a removeLock call.  We should add it; it seems like if we ever
hit this error case, the settings lock queue would get stuck.  It's not
clear whether this is actually the cause of bug 1105511, so I'm putting
it in a separate bug.

Comment 2

[:gerard-majax]

That would make sense, I can't even explain why I have not been able to see it before, the error sequencing in finalizeSets() is obviously always doing this, except for this case. I'm wondering if maybe we should not do this removeLock() call in the promise handler rather than at this level, because I see a lot of calls.

I'll use the plane to PDX to investigate this :)

Comment 3

[:gerard-majax]

With a quick hack I can confirm the blockage ... and the fix proposed seems to be working as expected.

Comment 4

[:gerard-majax]

Attachment: Exposing the issue

With this change, we will for the issue for some lock.
Expected behavior is:
 - without the fix, the queue will be blocked for ever
 - with the fix, requests between #400 and #420 will get broken, but further will work

Tested on a Mulet, this gets you to homescreen, and gives time to unlock. Then we can see that Settings application is broken when you try to toggle some. Until you gover the level of request #420.

Comment 5

[:gerard-majax]

[Blocking Requested - why for this release]: This is affecting v2.1 for sure, since it's in the set of patches that got uplifted (bug 900551).

Comment 6

[:gerard-majax]

Kyle, just FYI, since it's due to bug 900551.

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Got it, thanks. Fix lgtm.

Comment 8

[:gerard-majax]

I'll need some help on the way we could test this: we have xpchsell unit test for this components, but I'd need to fake the settings locking code to make getObjectStore() fail.

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8530569 [details] [diff] [review]
Call removeLock in case where the call was missing

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

Taking review on this and r+'ing. Don't think it'll fix bug 1105511 (since this won't throw an exception) but it's good to have anyways.

Comment 10

[Carsten Book [:Tomcat]]

Hi, can you provide a try run for this change thanks!

Comment 11

[:gerard-majax]

https://tbpl.mozilla.org/?tree=Try&rev=2775049ecfad

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d920332c6ce6

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d920332c6ce6

Comment 14

[:gerard-majax]

Comment on attachment 8530569 [details] [diff] [review]
Call removeLock in case where the call was missing

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 #): Settings OOP
User impact if declined: major, settings api completely blocked once triggered
Testing completed: exposed one case and tested the fix on device
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/9435fbeb2fda