MozillaWiki

FirefoxOS/Device Storage/Worker Design: Difference between revisions

Page Discussion

FirefoxOS/Device Storage/Worker Design (view source)

Revision as of 16:40, 19 August 2015

4,937 bytes added ,  19 August 2015
add references to bugs
(add notes about file watcher notifications)
(add references to bugs)
 
(6 intermediate revisions by the same user not shown)
Line 1: Line 1:
= Phase 1 =
= Phase 1 =


== Expose DOMRequest to workers ==
== Expose DOMRequest and DOMCursor to workers ==


See [https://bugzilla.mozilla.org/show_bug.cgi?id=1167650 bug 1167650].
See [https://bugzilla.mozilla.org/show_bug.cgi?id=1167650 bug 1167650].


DOMRequest is currently only accessible on the main thread. It be supported by workers we must:
DOMRequest and DOMCursor are currently only accessible on the main thread. It be supported by workers we must:


# Allow it to be constructed using nsIGlobalObject* (fed into DOMEventTargetHelper) in addition to nsPIDOMWindow*. The underlying Promise needs that to get a JS context and check state.
# Allow DOMRequest/DOMCursor to be constructed using nsIGlobalObject* (fed into DOMEventTargetHelper) in addition to nsPIDOMWindow*. The underlying Promise needs that to get a JS context and check state.
# DOMRequestService::FireSuccessAsync (FireSuccessAsyncTask by extension) and DOMRequestService::FireErrorAsync must be changed to dispatch to the current thread and remove the assert checks for main thread. mozilla::ThreadsafeAutoSafeJSContext should be switched for AutoJSAPI and Init(nsIGlobalObject*) (gotten from DOMRequest::GetParentObject); this will assert that the current thread is owning thread.
# DOMRequestService::FireSuccessAsync (FireSuccessAsyncTask by extension) and DOMRequestService::FireErrorAsync must be changed to dispatch to the current thread and remove the assert checks for main thread. mozilla::ThreadsafeAutoSafeJSContext should be switched for AutoJSAPI and Init(nsIGlobalObject*) (gotten from DOMRequest::GetParentObject); this will assert that the current thread is owning thread.
# Add Exposed=(Window,Worker) to the WebIDL definition.
# Add Exposed=(Window,Worker) to the WebIDL definitions.


== Ensure volume service is thread safe ==
== Ensure volume service is thread safe ==
Line 25: Line 25:


== Abstract use of preferences to dedicated module ==
== Abstract use of preferences to dedicated module ==
See [https://bugzilla.mozilla.org/show_bug.cgi?id=1186273 bug 1186273].


Preferences can only be accessed on the main thread. Workers should access a cached, thread safe version of that in a dedicated module. This should merge uses of preferences throughout the code, including GlobalDirs and OverrideRootDir.
Preferences can only be accessed on the main thread. Workers should access a cached, thread safe version of that in a dedicated module. This should merge uses of preferences throughout the code, including GlobalDirs and OverrideRootDir.
Line 47: Line 49:


The string bundle service can only be accessed on the main thread and should be initialized as part of the preferences.
The string bundle service can only be accessed on the main thread and should be initialized as part of the preferences.
== Observers ==
See [https://bugzilla.mozilla.org/show_bug.cgi?id=1186273 bug 1186273].
The observer service can only be accessed on the main thread. Workers should access a cached, thread safe version of that in a dedicated module. This should merge uses of observers throughout the code, including FileUpdateDispatcher.
This module (ObserverManager) should be a singleton, created by nsLayoutStatics::Initialize and destroyed via ClearOnShutdown. If the singleton is missing, all device storage APIs should fail (rather than attempt to initialize given they may not be on the main thread).
When an nsDOMDeviceStorage object is created/destroyed, it should register/deregister as a listener. Any ObserverManager::Observe notifications should be proxied on the current thread to the nsDOMDeviceStorage objects, after doing any common work such as querying the volume manager.
The following events will need to be pushed to the nsDOMDeviceStorage objects on the worker threads:
* nsPref:changed
* volume-state-changed
* file-watcher-update
* disk-space-watcher
The following events may be processed inside the ObserverManager:
* file-watcher-notify
* download-watcher-notify
There is a special case for event listeners as presently it adds/removes an observer if a system listener is added to nsDOMDeviceStorage (see ::AddSystemEventListener and ::RemoveEventListener). Since nsDOMDeviceStorage (and going forward the ObserverManager) already listen on those events, this code should be removed as there is no need for a special case (additionally, there does not appear to be any system listeners using nsDOMDeviceStorage, so this is dead code.)
Rather than accept nsDOMDeviceStorage objects directly, it should define an interface which it implements so that other objects (i.e. DeviceStorageAreaListener) may reuse the same module to receive volume state change notifications.


== Consolidate and cache use of permissions ==
== Consolidate and cache use of permissions ==
See [https://bugzilla.mozilla.org/show_bug.cgi?id=1171170 bug 1171170].


When checking the permissions, we must run on main thread to possibly put up allow/deny dialog and to query the parent process (if a child). To minimize delegating to the main thread from workers, we should cache the permission check result. In addition this should reduce the latency of device storage APIs due to removing an extra round trip between the parent and child processes.
When checking the permissions, we must run on main thread to possibly put up allow/deny dialog and to query the parent process (if a child). To minimize delegating to the main thread from workers, we should cache the permission check result. In addition this should reduce the latency of device storage APIs due to removing an extra round trip between the parent and child processes.
Line 68: Line 96:
This is a special case for adding an event listener. While one may register without any permissions, one cannot receive the events unless one is permitted (otherwise the event is dropped). Presently we overload DOMEventTargetHelper::AddEventListener to trigger this permission check, which has several variations. We should switch to overloading DOMEventTargetHelper::EventListenerWasAdded which is meant for this purpose in derived classes and is triggered no matter how the listener is added.
This is a special case for adding an event listener. While one may register without any permissions, one cannot receive the events unless one is permitted (otherwise the event is dropped). Presently we overload DOMEventTargetHelper::AddEventListener to trigger this permission check, which has several variations. We should switch to overloading DOMEventTargetHelper::EventListenerWasAdded which is meant for this purpose in derived classes and is triggered no matter how the listener is added.


== Observers ==
== Cycle collected reference counting ==


The observer service can only be accessed on the main thread. Workers should access a cached, thread safe version of that in a dedicated module. This should merge uses of observers throughout the code, including FileUpdateDispatcher.
See [https://bugzilla.mozilla.org/show_bug.cgi?id=1171170 bug 1171170].


This module (ObserverManager) should be a singleton, created by nsLayoutStatics::Initialize and destroyed via ClearOnShutdown. If the singleton is missing, all device storage APIs should fail (rather than attempt to initialize given they may not be on the main thread).
nsDOMDeviceStorage, nsDOMDeviceStorageCursor, DOMRequest and DeviceStorageRequest are all cycle collected objects. The reference counting for cycle collected objects is not threadsafe (and this is unlikely to change). As such great care must be used when moving references across thread boundaries, in addition to the fact we must guarantee they are freed in the owning thread context.


When an nsDOMDeviceStorage object is created/destroyed, it should register/deregister as a listener. Any ObserverManager::Observe notifications should be proxied on the current thread to the nsDOMDeviceStorage objects, after doing any common work such as querying the volume manager.
The only class that can potentially change is DeviceStorageRequest. The cycle collection dependencies are from subclassing nsIContentPermissionRequest and holding DOMRequest, nsPIDOMWindow, and Blob references. The permission changes above will remove nsIContentPermissionRequest and the nsPIDOMWindow reference. The Blob reference may be converted into the threadsafe BlobImpl reference (which we already do later in the process). DOMRequest is more problematic.


The following events will need to be pushed to the nsDOMDeviceStorage objects on the worker threads:
Today the code carefully moves the DOMRequest from object to object with use of .forget(). If used exclusively until we post the result on the owning thread, this will prevent modifying the reference count off the owning thread. In addition to auditing the reference counting, we would also need to use NS_ProxyRelease in all destructors of the holding classes, as well as hold a reference to the owning thread, to guarantee it is freed on the correct thread if for whatever reason the DOMRequest reference was not passed off via .forget().
* nsPref:changed
* volume-state-changed
* file-watcher-update
* disk-space-watcher
 
The following events may be processed inside the ObserverManager:
* file-watcher-notify
* download-watcher-notify


There is a special case for event listeners as presently it adds/removes an observer if a system listener is added to nsDOMDeviceStorage (see ::AddSystemEventListener and ::RemoveEventListener). Since nsDOMDeviceStorage (and going forward the ObserverManager) already listen on those events, this code should be removed as there is no need for a special case (additionally, there does not appear to be any system listeners using nsDOMDeviceStorage, so this is dead code.)
The alternative is centralize all references to DOM objects in a threadsafe refcounting request manager. The manager would supply the rest of the module with opaque handles identifying the DOMRequest objects. When any other object needs to interact with the DOM objects, the manager would marshal access appropriately. For posting success/error responses, it would post to the owning thread with the desired value, do any necessary JS transforms, trigger the appropriate action (FireSuccess/FireError/FireDone) and if complete, release its reference to the DOM object. The lifecycle of a single class, which is independent of the specific operations being performed, would be much easier to manage and guarantee correctness.


Rather than accept nsDOMDeviceStorage objects directly, it should define an interface which it implements so that other objects (i.e. DeviceStorageAreaListener) may reuse the same module to receive volume state change notifications.
== Device storage IPC API with child-parent processes ==
 
== Used space cache (DeviceStorageUsedSpaceCache) ==
 
This is only accessed on parent process, so there is no child process concerns. However since workers can also exist on the parent, it needs to be made thread safe. It may continue to be initialized on demand since it has no dependencies on the main thread. All operations besides initialization appear to run on a dedicated IO thread. As such, only the ::CreateOrGet method needs to be protected via a mutex.


== Device storage IPC API with child-parent processes ==
See [https://bugzilla.mozilla.org/show_bug.cgi?id=1171170 bug 1171170].


PContent is the IPC object which marshalls access to the device storage IPC APIs. This is a singleton and can only be accessed on the main thread.
PContent is the IPC object which marshalls access to the device storage IPC APIs. This is a singleton and can only be accessed on the main thread.
Line 107: Line 123:


Receiving the responses happens in DeviceStorageRequestChild::Recv__delete__. This will need to create a runnable to proxy the response to the owning thread of the DeviceStorageRequest.
Receiving the responses happens in DeviceStorageRequestChild::Recv__delete__. This will need to create a runnable to proxy the response to the owning thread of the DeviceStorageRequest.
== Used space cache (DeviceStorageUsedSpaceCache) ==
See [https://bugzilla.mozilla.org/show_bug.cgi?id=1196321 bug 1196321].
This is only accessed on parent process, so there is no child process concerns. However since workers can also exist on the parent, it needs to be made thread safe. It may continue to be initialized on demand since it has no dependencies on the main thread. All operations besides initialization appear to run on a dedicated IO thread. As such, only the ::CreateOrGet method needs to be protected via a mutex.
== MIME service ==
See [https://bugzilla.mozilla.org/show_bug.cgi?id=1196315 bug 1196315].
The MIME type of a file is queried for each file when an enumeration operation is requested. The MIME service can only be accessed on the main thread; this is problematic given that this would presently require a context switch to and from each time the user calls continue on our enumeration cursor. nsExternalHelperAppService::GetTypeFromFile extracts the file extension and check then checks that against the numerous sources of MIME information (consistent with the IDL description). We should switch to extracting the file extension ourselves, use nsExternalHelperAppService:::GetTypeFromExtension to get the MIME type on the main thread for unknown extensions as is done today, and cache the result for the duration of the enumeration cursor. This balances worker performance (i.e. very likely that the number of files greatly exceeds the number of unique extensions) with accuracy (i.e. by caching only for a particular cursor request, we allow the underlying service to discover new MIME types by addition of plugins, etc if the enumeration is reattempted).
It is also queried when a file is added (prior to issuing the request) via nsDOMDeviceStorage::Add in order to determine the correct file extension. This should be moved into the create request itself, after the permission check, and before the file creation itself or sending the request to the parent process (if a child process). Note that while nsDOMDeviceStorage::Add indicates the file will be rejected by nsDOMDeviceStorage::Add(OrAppend)Named if the given extension is empty, the check is actually performed just before creating the WriteFileEvent object in DeviceStorageRequest::Allow.


== Expose Device Storage to workers ==
== Expose Device Storage to workers ==
See [https://bugzilla.mozilla.org/show_bug.cgi?id=1196322 bug 1196322].


The appropriate WebIDL Exposure tags should be added to all device storage interfaces and the Navigator device storage partial interfaces.
The appropriate WebIDL Exposure tags should be added to all device storage interfaces and the Navigator device storage partial interfaces.
Line 115: Line 147:


DeviceStorageAreaListener should be updated to listener on the ObserverManager, same as nsDOMDeviceStorage, in order to receive the volume state change messages.
DeviceStorageAreaListener should be updated to listener on the ObserverManager, same as nsDOMDeviceStorage, in order to receive the volume state change messages.
nsDOMDeviceStorage must maintain a reference to the owning thread and ensure that all DOM interacting code runs on said thread. It must also ensure that all references to the window are replaced with either the nsIGlobalScript reference (i.e. nsDOMDeviceStorage::GetOwnerGlobal()) for DOMCursor and DOMRequest objects, nsDOMDeviceStorage for objects that accept an nsISupports reference as the parent object (i.e. Blob::Create) and nullptr for the remainder.


All existing device storage test cases should be updated/cloned to run in both a worker and the main thread contexts.
All existing device storage test cases should be updated/cloned to run in both a worker and the main thread contexts.
Aosmond
24

edits

Retrieved from "https://wiki.mozilla.org/Special:MobileDiff/1077088...1090834"