1196315 - [DeviceStorage] Consolidate and cache accesses to MIME service

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Andrew Osmond [:aosmond] (he/him)]

See https://wiki.mozilla.org/FirefoxOS/Device_Storage/Worker_Design#MIME_service.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Attachment: [gecko] consolidate access to MIME service, v1

Patch does not contain caching, there is actually not much point since the cursor request is the only one which benefits, and when in the child process (primary use case), it will need to post to the main thread *anyways* to send the open request. Caching can be revisited if/when cursors are updated to operate on blocks of files internally and/or when the IPC can be accessed off main thread.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=675901d65d6e

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

Attachment: [gecko] consolidate access to MIME service, v2

Fix how it should have returned early in ::AddOrAppendNamed.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=35f6c771b9ff

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Whoops, patch I paired it with had trouble, try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe38d86b57d3

Comment 5

[Dave Hylands [:dhylands]]

Comment on attachment 8662115 [details] [diff] [review]
[gecko] consolidate access to MIME service, v2

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

Just a few cleanups. r=me with those done.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +1893,5 @@
>  class DeviceStorageCreateRequest final
>    : public DeviceStorageRequest
>  {
>  public:
> +  using DeviceStorageRequest::Initialize;

what does this do?

It's already declared public in the base class.

@@ +1967,5 @@
> +    // will post an onerror to the requestee.
> +
> +    // possible race here w/ unique filename
> +    char buffer[32];
> +    NS_MakeRandomString(buffer, ArrayLength(buffer) - 1);

Shouldn't this use DeviceStorageFile::CreateUnique? Or factor out some functionality from that so that we don't have a race/duplicate filename here.

I see you're just copying the original code here, but we might as well fix it properly.

@@ +3086,3 @@
>    nsCOMPtr<nsIRunnable> r;
>  
> +  if (!aPath.IsEmpty() && IsFullPath(aPath)) {

IsFullPath already checks for aPath.Length() > 0 so why the extra check?

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

Attachment: [gecko] consolidate access to MIME service, v3 [carries r=dhylands]

(In reply to Dave Hylands [:dhylands] from comment #5)
> Comment on attachment 8662115 [details] [diff] [review]
> [gecko] consolidate access to MIME service, v2
> 
> Review of attachment 8662115 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just a few cleanups. r=me with those done.
> 
> ::: dom/devicestorage/nsDeviceStorage.cpp
> @@ +1893,5 @@
> >  class DeviceStorageCreateRequest final
> >    : public DeviceStorageRequest
> >  {
> >  public:
> > +  using DeviceStorageRequest::Initialize;
> 
> what does this do?
> 
> It's already declared public in the base class.
> 

Compile breakage. I think it is because I overload it. Had to do the same thing in DeviceStorageOpenRequest.

> @@ +1967,5 @@
> > +    // will post an onerror to the requestee.
> > +
> > +    // possible race here w/ unique filename
> > +    char buffer[32];
> > +    NS_MakeRandomString(buffer, ArrayLength(buffer) - 1);
> 
> Shouldn't this use DeviceStorageFile::CreateUnique? Or factor out some
> functionality from that so that we don't have a race/duplicate filename here.
> 
> I see you're just copying the original code here, but we might as well fix
> it properly.
> 

Done. Although it still uses NS_MakeRandomString, but ::CreateUnique will ensure that is actually unique and yield a new DeviceStorageFile instead of reusing the old one.

> @@ +3086,3 @@
> >    nsCOMPtr<nsIRunnable> r;
> >  
> > +  if (!aPath.IsEmpty() && IsFullPath(aPath)) {
> 
> IsFullPath already checks for aPath.Length() > 0 so why the extra check?

Ugh, good point :).

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Andrew Osmond [:aosmond] from comment #6)
> Created attachment 8664747 [details] [diff] [review]
> [gecko] consolidate access to MIME service, v3 [carries r=dhylands]
> 
> (In reply to Dave Hylands [:dhylands] from comment #5)
> > Comment on attachment 8662115 [details] [diff] [review]
> > [gecko] consolidate access to MIME service, v2
> > 
> > Review of attachment 8662115 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Just a few cleanups. r=me with those done.
> > 
> > ::: dom/devicestorage/nsDeviceStorage.cpp
> > @@ +1893,5 @@
> > >  class DeviceStorageCreateRequest final
> > >    : public DeviceStorageRequest
> > >  {
> > >  public:
> > > +  using DeviceStorageRequest::Initialize;
> > 
> > what does this do?
> > 
> > It's already declared public in the base class.
> > 
> 
> Compile breakage. I think it is because I overload it. Had to do the same
> thing in DeviceStorageOpenRequest.
> 

Err that is to say, because I *override* it *and* call the original function inside the overridden function.

Comment 8

[Pulsebot]

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

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/672e0e69482e