1063877 - Change nsVolumeService to use sync IPC

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Dave Hylands [:dhylands]]

Currently, the nsVolumeService sends an async Broadcast request to the parent, which in turn sends back an async response about the volumes which are available.

This changes the nsVolumeService on the child to issue a sync IPC call to get the initial set of volumes.

There are a number of advantages to this:
1 - Only the child in question gets the volume information
2 - It eliminates some potential races when the async method.

Comment 1

[Dave Hylands [:dhylands]]

Attachment: Use sync IPC to get initial list of volumes

Comment 2

[Dave Hylands [:dhylands]]

Attachment: Use sync IPC to get initial list of volumes v2

I think that this will fix the root cause in bug 1062541.

Comment 3

[Dave Hylands [:dhylands]]

Justin - can you try this patch and see if it also fixes bug 1058016 ?

Comment 4

[Dave Hylands [:dhylands]]

https://tbpl.mozilla.org/?tree=Try&rev=1325743c6cd4

Comment 5

[Dave Hylands [:dhylands]]

Attachment: Use sync IPC to get initial list of volumes v3

Let's try this again

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

Comment 6

[Justin D'Arcangelo [:justindarc]]

(In reply to Dave Hylands [:dhylands][on PTO - back Sep 11] from comment #3)
> Justin - can you try this patch and see if it also fixes bug 1058016 ?

Dave: Looks good! I seem unable to reproduce Bug 1058016 with this patch applied.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8485362 [details] [diff] [review]
Use sync IPC to get initial list of volumes v3

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

It feels dirty to block, but ok.

::: dom/system/gonk/nsIVolumeService.idl
@@ +10,3 @@
>  
>  [scriptable, uuid(cab99ab4-542e-4387-bd40-db6ef30e4f5f)]
>  interface nsIVolumeService : nsISupports

change the uuid

Comment 9

[Dave Hylands [:dhylands]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #8)
> Comment on attachment 8485362 [details] [diff] [review]
> Use sync IPC to get initial list of volumes v3
> 
> Review of attachment 8485362 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It feels dirty to block, but ok.

Well it's at most once per the lifetime of each process.

> ::: dom/system/gonk/nsIVolumeService.idl
> @@ +10,3 @@
> >  
> >  [scriptable, uuid(cab99ab4-542e-4387-bd40-db6ef30e4f5f)]
> >  interface nsIVolumeService : nsISupports
> 
> change the uuid

Whoops - will do.

Comment 10

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/integration/b2g-inbound/rev/01fe5d23d233

Comment 11

[Dave Hylands [:dhylands]]

Backed out due to breaking emulator tests:
https://hg.mozilla.org/integration/b2g-inbound/rev/ee4e51c71b03

Comment 12

[Dave Hylands [:dhylands]]

It looks like I removed an if I shouldn't have. Added it back. Try is now green:
https://tbpl.mozilla.org/?tree=Try&rev=53094dbd65d7

so I'll reland.

Comment 13

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ffd263ab3bc8

Comment 14

[Dave Hylands [:dhylands]]

Comment on attachment 8485362 [details] [diff] [review]
Use sync IPC to get initial list of volumes v3

Approval Request Comment
[Feature/regressing bug #]: Always been present
[User impact if declined]: Sometimes, device storage volumes appear to be missing
[Describe test coverage new/current, TBPL]: existing TBPL
[Risks and why]: I think that the risks are fairly low. The change eliminates a race condition in how the initial volumes are determined.
[String/UUID change made/needed]: none

Comment 15

[Dave Hylands [:dhylands]]

Comment on attachment 8485362 [details] [diff] [review]
Use sync IPC to get initial list of volumes v3

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

Approval Request Comment
[Feature/regressing bug #]: Always been present
[User impact if declined]: Sometimes, device storage volumes appear to be missing
[Describe test coverage new/current, TBPL]: existing TBPL
[Risks and why]: I think that the risks are fairly low. The change eliminates a race condition in how the initial volumes are determined.
[String/UUID change made/needed]: none

I'm asking for 2.0 approval since I believe that this patch fixes the root cause of bug 1062541

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 17

[Hema Koka [:hema]]

Reason: Blocking a blocker, See bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1068162 leading to unstable SDcard detection (based on stability testing)

Comment 18

[Dave Hylands [:dhylands]]

Adding fabrice and bhavana for approval

Comment 19

[bhavana bajaj [:bajaj]]

Switching the approval to highest impacted branch which 2.0 and to fix https://bugzilla.mozilla.org/show_bug.cgi?id=1062541

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9ef7e2446ea8
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/96169f9e554f

Comment 21

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/96169f9e554f