Closed Bug 1063877 Opened 6 years ago Closed 6 years ago

Change nsVolumeService to use sync IPC

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(blocking-b2g:2.0+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S5 (26sep)
blocking-b2g 2.0+
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: dhylands, Assigned: dhylands)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
I think that this will fix the root cause in bug 1062541.
Attachment #8485340 - Attachment is obsolete: true
Attachment #8485341 - Flags: review?(khuey)
Justin - can you try this patch and see if it also fixes bug 1058016 ?
Flags: needinfo?(jdarcangelo)
Assignee: nobody → dhylands
Let's try this again

 https://tbpl.mozilla.org/?tree=Try&rev=a7980bd77552
Attachment #8485341 - Attachment is obsolete: true
Attachment #8485341 - Flags: review?(khuey)
Attachment #8485362 - Flags: review?(khuey)
Target Milestone: --- → 2.1 S4 (12sep)
(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.
Flags: needinfo?(jdarcangelo)
Duplicate of this bug: 1066489
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
Attachment #8485362 - Flags: review?(khuey) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Backed out due to breaking emulator tests:
https://hg.mozilla.org/integration/b2g-inbound/rev/ee4e51c71b03
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 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
Attachment #8485362 - Flags: approval-mozilla-aurora?
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
Attachment #8485362 - Flags: approval-mozilla-b2g32?
https://hg.mozilla.org/mozilla-central/rev/ffd263ab3bc8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: 2.1 S4 (12sep) → 2.1 S5 (26sep)
Reason: Blocking a blocker, See bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1068162 leading to unstable SDcard detection (based on stability testing)
blocking-b2g: --- → 2.1+
Adding fabrice and bhavana for approval
Flags: needinfo?(fabrice)
Flags: needinfo?(bbajaj)
Flags: needinfo?(fabrice)
Switching the approval to highest impacted branch which 2.0 and to fix https://bugzilla.mozilla.org/show_bug.cgi?id=1062541
blocking-b2g: 2.1+ → 2.0+
Flags: needinfo?(bbajaj)
Attachment #8485362 - Flags: approval-mozilla-b2g32?
Attachment #8485362 - Flags: approval-mozilla-b2g32+
Attachment #8485362 - Flags: approval-mozilla-aurora?
Attachment #8485362 - Flags: approval-mozilla-aurora+
Depends on: 1112989
Depends on: 1126119
You need to log in before you can comment on or make changes to this bug.