1171170 - [DeviceStorage] Consolidate and cache use of permissions

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

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

Comment 1

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

Attachment: WIP - Consolidate/cache use of permissions, v1

Streamlined permission checks, eliminated special permission path for enumerate. Still need to test and cleanup.

Comment 2

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

Attachment: Consolidate/cache use of permissions, v2

Comment 3

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

Comment on attachment 8615279 [details] [diff] [review]
Consolidate/cache use of permissions, v2

Clearing review, I think there are some improvements I can do on all of the runnables littering the code, especially the ones I need (go C++11x!).

Comment 4

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

Attachment: Consolidate/cache use of permissions, v3

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

Now with lambdas!

Comment 5

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

Attachment: WIP - Refactor device storage (including permission caching), v1

Comment 6

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

(In reply to Andrew Osmond [:aosmond] from comment #5)
> Created attachment 8616796 [details] [diff] [review]
> WIP - Refactor device storage (including permission caching), v1

Untested (but building) WIP as an alternative to the other permission patch. A bit large and sweeping but provides a number of benefits:

-- DeviceStoragePermissionRequest is now the owner of querying the permission system. The results are cached so it should only be used up to 4 times per nsDOMDeviceStorage object. This caching saves roundtrips to the parent process for each operation and reduces the main thread dependency for a worker bound object.

-- DeviceStorageRequestManager is a new threadsafe class. It is the only other class (besides nsDOMDeviceStorage) which can hold references to worker thread only, cycle collected objects (i.e. nsDOMDeviceStorage, DOMRequest, nsDOMDeviceStorageCursor). Its responsibility is to marshal access to these objects, such that they are always created, destroyed and modified on the worker thread. However since it holds references to cycle collected objects, it must be very careful to free itself and its children when no longer needed.

-- DeviceStorageRequest is no longer cycle collected. The only cycle collected classe are nsDOMDeviceStorage and DeviceStoragePermissionRequest (those cannot change). That makes DeviceStorageRequest safe (from a refcounting perspective) to use with any thread. This has been achieved by holding only a numerical ID for the actual DOMRequest/Cursor, holding the underlying BlobImpl instead of Blob and removing all permission checks from this class.

-- DeviceStorageRequest is now what requests revolve around (apt name, that). All helper event classes (e.g. WriteFileEvent, etc) have been merged with it, such that they subclass DeviceStorageRequest and provide the operation specific implementation details. This is broken down into the actual operation on the parent process (::Run) and packaging the parameters to send to the parent process if a child process (::CreateSendParams). This also has the advantage of eliminating the giant switch statement :).

-- This in theory allows us to move towards harmonizing the duplicate implementation of operations in DeviceStorageRequestParent. One could create a DeviceStorageRequest with DeviceStorageParentRequestManager object which captures the results for shuttling back to the child.

Comment 7

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

Comment on attachment 8615394 [details] [diff] [review]
Consolidate/cache use of permissions, v3

Clearing review but I'll leave it up for comparison purposes for the moment.

Comment 8

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

Comment on attachment 8616796 [details] [diff] [review]
WIP - Refactor device storage (including permission caching), v1

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

Some thoughts...

::: dom/devicestorage/DeviceStorageRequestChild.cpp
@@ +61,5 @@
>        nsString fullPath;
> +      file->GetFullPath(fullPath);
> +      descriptor->mDSFile = file;
> +      descriptor->mFileDescriptor = r.fileDescriptor();
> +      mRequest->Resolve(fullPath);

I'm not sure I fully appreciate what is happening here (from the original code). It gives "fullPath" as the result but it populates the descriptor before doing that. Nothing else is done with the descriptor (as far as I can tell) and it is eventually freed. The destructor doesn't do anything special. What is the purpose?

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +2952,5 @@
> +      rv = mainThread->Dispatch(r, NS_DISPATCH_NORMAL);
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        rv = aRequest->Reject(POST_ERROR_EVENT_UNKNOWN);
> +      }
> +      NS_ProxyRelease(mainThread, r.forget().take());

I believe this is a race condition that needs to be accounted for, but it does make me feel paranoid.

@@ +3457,5 @@
> +  nsRefPtr<DOMRequest> request;
> +  uint32_t id = CreateDOMRequest(getter_AddRefs(request));
> +  aRv = mManager->Reject(id, aReason);
> +  return request.forget();
> +}

Use of these is a bit clunky but I did not want to disturb the pass/failure paths at all and potentially impact the API users. Ideally we would always return a DOMRequest, fail via FireError and avoid throwing errors unless absolutely necessary. This would bring us closer in behaviour to promises, where even if one throws an error while returning a nullptr promise, it will (in another layer) implicitly create a promise for the JS user and rejects it.

@@ +4000,5 @@
>  
>  nsresult
>  nsDOMDeviceStorage::Notify(const char* aReason, DeviceStorageFile* aFile)
>  {
> +  if (!HasListenersFor(NS_LITERAL_STRING(STORAGE_CHANGE_EVENT))) {

Oops, this should be checking the mPermissionCache *not* checking if there are listeners.

@@ +4241,5 @@
> +    /* TODO: Why do we have two means of converting a Blob to a JS::Value?
> +       The implementation for Resolve(BlobImpl) is much simpler... */
> +    JS::RootedValue value(jsapi.cx(),
> +                          nsIFileToJsval(jsapi.cx(), global, file));
> +    self->ResolveInternal(aId, value);

Note how the ::Resolve function below for BlobImpl is very simple. I pulled that code from DeviceStorageRequestChild.cpp. nsIFileToJsval is doing the same thing (it creates a Blob) but after that the methods diverge and I don't really follow the existing code. That said, I didn't touch it in fear of breaking it :). Why are they different?

Comment 9

[Dave Hylands [:dhylands]]

Comment on attachment 8616796 [details] [diff] [review]
WIP - Refactor device storage (including permission caching), v1

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

::: dom/devicestorage/DeviceStorageRequestChild.cpp
@@ +61,5 @@
>        nsString fullPath;
> +      file->GetFullPath(fullPath);
> +      descriptor->mDSFile = file;
> +      descriptor->mFileDescriptor = r.fileDescriptor();
> +      mRequest->Resolve(fullPath);

My recollection of this is that the caller provides the memory to place the FileDescriptor object.

When the child calls CreateFileDescriptor here:
https://dxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraControl.cpp?from=DOMCameraControl.cpp#765
it allocates the file descriptor, which in turn gets stored in the DeviceStorageRequestChild

https://dxr.mozilla.org/mozilla-central/source/dom/devicestorage/nsDeviceStorage.cpp#3025

That then comes back from the parent  through the code where this comment is:
https://dxr.mozilla.org/mozilla-central/source/dom/devicestorage/DeviceStorageRequestChild.cpp#87-101

So the key is that mDSFileDescriptor (in the original code) is a pointer to the caller's allocated memory.

Comment 10

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

Attachment: WIP - Refactor device storage (including permission caching), v2

Add some basic logging, fix regressions. Device boots, gallery/video/camera are working.

Comment 11

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

Attachment: WIP - Refactor device storage (including permission caching), v3

Err, attach a complete patch this time.

Comment 12

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

Attachment: WIP - Refactor device storage (including permission caching), v4

Change/add asserts, extra logging, some minor optimizations.

Comment 13

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

Attachment: WIP - Refactor device storage (including permission caching), v5

Minor optimizations for cursors -- preallocate array capacity when assembling file list on child, avoid posting to main thread for IPC call if already on it.

Comment 14

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

Attachment: WIP - Part 2. Merge BlobImpl to JS conversions, v1

The old JS code was very confusing and magical to me. I'm not sure why it would not convert editable files given the blob is marked as mutable when created anyways? Do you know if this is tested today or if there is a way to test this? (Presumably it would have to be with the desktop code in a single process mode...)

Comment 15

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

try for the two WIPs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7882fd298479

Comment 16

[Dave Hylands [:dhylands]]

(In reply to Andrew Osmond [:aosmond] from comment #14)
> Created attachment 8617371 [details] [diff] [review]
> WIP - Part 2. Merge BlobImpl to JS conversions, v1
> 
> The old JS code was very confusing and magical to me. I'm not sure why it
> would not convert editable files given the blob is marked as mutable when
> created anyways? Do you know if this is tested today or if there is a way to
> test this? (Presumably it would have to be with the desktop code in a single
> process mode...)

It has to do with file locking. At the time that code was written, the file locking for out-of-process didn't work.

It landed a few months ago, and just needs to be plumbed in.

Comment 17

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

try for the two WIPs after (hopefully) fixing non-B2G compile errors: https://treeherder.mozilla.org/#/jobs?repo=try&revision=449399e63735

Comment 18

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

Attachment: WIP - Refactor device storage (including permission caching), v6

try on WIP again, fixing a few bugs (oops, cursor requests don't have to complete apparently...): https://treeherder.mozilla.org/#/jobs?repo=try&revision=23afc74c6ae0

Comment 19

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

Attachment: WIP - Refactor device storage (including permission caching), v7

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

- Fix Windows build, logging would be nice there (hopefully)
- Fix some ::Shutdown related bugs (failing tests due to requests getting cleaned up when before they were dropped, prevent new requests coming in after shutdown, etc)
- Make manager only hold onto a reference to device storage while there are pending requests and ::Shutdown hasn't been called; it is only used to store permission results (safe to discard) and when creating new DOMRequest objects (but that's where we add a reference). nsIGlobalObject now retrieved from the DOMRequest itself at Resolve time.
- Make delete requests run on a stream transport thread instead of worker (whoops)
- Fix missing MIME type for open requests
- Fix how cursor FireError did not reset mDone and mResult (revealed by ::Shutdown but in theory could be caused today if the get IPC call failed in Continue)
- Remove trailing whitespace.

Comment 20

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

Attachment: Part 2. Merge BlobImpl to JS conversions, v2

Refresh patch as it goes on top of the other WIP.

Comment 21

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

Attachment: WIP - Refactor device storage (including permission caching), v8

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

Looks like they are all passing.

Comment 22

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

Attachment: WIP - Refactor device storage (including permission caching), v9

Make some optimizations discussed on etherpad by reducing signalling when not necessary. This brings the main thread signalling overhead back to where it was, with also the reduction of one extra dispatch if we hit the permission cache as well. DeviceStorageRequest::Allow can now run in both the worker and main thread context (depending on if we hit the permission cache).

Comment 23

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

Attachment: WIP - Part 2. Merge BlobImpl to JS conversions, v3

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=14f8f3376324

Comment 24

[Dave Hylands [:dhylands]]

Comment on attachment 8615394 [details] [diff] [review]
Consolidate/cache use of permissions, v3

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

I wasn't sure if this file was still valid or not - I gave it a look through anyways.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +441,5 @@
> +nsresult
> +DeviceStorageTypeChecker::GetAccessForRequest(
> +  const DeviceStorageRequestType aRequestType, nsACString& aAccessResult)
> +{
> +  static const char *names[] = { "read", "write", "create", "undefined" };

nit * should snuggle char

I think that we should put a comment in DeviceStorage.h that says that any changes to the DEVICE_STORAGE_ACCESS_xxx (adding/removing/modidying) also requires a corresponding change to this function.

I'm always nervous when a 1:1 relationship is required and they're not in close proximity to each other in the source.

@@ +443,5 @@
> +  const DeviceStorageRequestType aRequestType, nsACString& aAccessResult)
> +{
> +  static const char *names[] = { "read", "write", "create", "undefined" };
> +  size_t access = GetAccessIndexForRequest(aRequestType);
> +  MOZ_ASSERT(access < MOZ_ARRAY_LENGTH(names));

I think that this should have an if to prevent out-of bound access when MOZ_ASSERT is disabled.

I'd be inclined to have any out of bounds access return "undefined".

@@ +1990,5 @@
>    nsRefPtr<DOMRequest> mRequest;
>    nsString mError;
>  };
>  
> +ContinueCursorEvent::ContinueCursorEvent(already_AddRefed<nsDOMDeviceStorageCursor> aCursor)

Hmm - not your fault, but I thought that ref counted stuff was passed as a bare pointer when passing as an argument.

@@ +2016,5 @@
>    }
>  
> +  while (mCursor->mFiles.Length() > 0) {
> +    nsRefPtr<DeviceStorageFile> file = mCursor->mFiles[0];
> +    mCursor->mFiles.RemoveElementAt(0);

I thought this was changed? Maybe this is an obsolte patch?

@@ +3245,5 @@
>      return NS_OK;
>    }
>  
> +
> +  NS_IMETHOD GetPrincipal(nsIPrincipal * *aRequestingPrincipal) override

nit: this func and 3 following - star placement

@@ +3377,5 @@
> +
> +nsresult
> +nsDOMDeviceStorage::DispatchToOwningThread(nsIRunnable* aRunnable)
> +{
> +  return mOwningThread->Dispatch(aRunnable, NS_DISPATCH_NORMAL);

Should this check if the current thread is the owning thread and then call NS_DispatchToCurrentThread? Or maybe that scenario would never arise?

@@ +4347,5 @@
> +    = new nsDOMDeviceStorageCursor(win, dsf, since);
> +
> +  nsCOMPtr<nsIRunnable> r;
> +  if (!dsf->IsSafePath()) {
> +    r = new PostErrorEvent(cursor, POST_ERROR_EVENT_PERMISSION_DENIED);

I'd like to see PostErorEvent log something when we get a Permission Denied. Right now things just silently fail.

Comment 25

[Dave Hylands [:dhylands]]

Comment on attachment 8621022 [details] [diff] [review]
WIP - Refactor device storage (including permission caching), v9

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

Looks good to me. I definitely like some of the restructuring that you've done.

::: dom/devicestorage/DeviceStorage.h
@@ +305,5 @@
> +  uint32_t CreateDOMRequest(DOMRequest** aRequest, ErrorResult& aRv);
> +  uint32_t CreateDOMCursor(DeviceStorageCursorRequest* aRequest,
> +                           nsDOMDeviceStorageCursor** aCursor,
> +                           ErrorResult& aRv);
> +  already_AddRefed<DOMRequest> CreateAndRejectDOMRequest(const char *aReason,

nit: * should snuggle char

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +2935,5 @@
> +    case nsIPermissionManager::ALLOW_ACTION:
> +      return aRequest->Allow();
> +    case nsIPermissionManager::DENY_ACTION:
> +      return aRequest->Cancel();
> +    case nsIPermissionManager::PROMPT_ACTION:

nit: I'd add a // fall-thru comment at the end of the line here just to indicate that it was intended

@@ +3049,5 @@
> +
> +nsDOMDeviceStorage::~nsDOMDeviceStorage()
> +{
> +  MOZ_ASSERT(IsOwningThread());
> +  sInstanceCount--;

We need to ensure that sInstanceCount increments and decrements are atomic.

Comment 26

[Dave Hylands [:dhylands]]

Comment on attachment 8621023 [details] [diff] [review]
WIP - Part 2. Merge BlobImpl to JS conversions, v3

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

lgtm

Comment 27

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

Attachment: Refactor device storage (including permission caching), v10

Rebased onto head.

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

Comment 28

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

Attachment: Refactor device storage (including permission caching), v11

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93a249e07f87

Fixed a reported memory leak in the tests caused by runnables dispatched to the main thread being dropped without being freed during shutdown. Now if shutdown is called, it will prevent resolve/reject runnables from being posted. This primarily affects long lived cursor requests.

Cleaned up remaining FIXMEs specific to this patch.

Comment 29

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

Comment on attachment 8652645 [details] [diff] [review]
Refactor device storage (including permission caching), v11

Try results look good to me, and sanity testing with the media apps hasn't revealed any problems. Should be ready for landing. I have changed a few things since rebasing it, mainly related to shutdown. It should not have a meaningful impact beyond avoiding memory leaks due to the main thread discarding runnables.

Comment 30

[Dave Hylands [:dhylands]]

Comment on attachment 8652645 [details] [diff] [review]
Refactor device storage (including permission caching), v11

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

::: dom/devicestorage/DeviceStorage.h
@@ +170,5 @@
>    static int InstanceCount() { return sInstanceCount; }
>  
>    static void InvalidateVolumeCaches();
>  
> +  static bool AllowWebAppsManage(nsPIDOMWindow* aWindow);

nit: I couldn't find any callers of this function. If this was added for some future use, then I think that the name needs some tweaking, otherwise it can be removed.

The way I read this, I read Allow as a verb, and I think that this is a query function, so I think it should be called IsAllowedWebAppsManage or AllowedWebAppsManage.

If this is really Allow as a verb then the name is fine.

Comment 31

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

Attachment: Refactor device storage (including permission caching), v12 [carries r=dhylands]

I removed AllowWebAppsManage and will readd/rename it in a future patch which actually requires it.

Comment 32

[Pulsebot]

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

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/36d8ea398cc4