Closed Bug 1186273 Opened 9 years ago Closed 9 years ago

[DeviceStorage] Move preferences and observers into dedicated module

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla43
Tracking Flags:
Tracking Status
firefox43 --- fixed

People

(Reporter: aosmond, Assigned: aosmond)

References

Details

(Whiteboard: [NG Device Storage])

Attachments

(2 files, 13 obsolete files)

Part 1. Move preferences and observers into dedicated threadsafe module. -- v5.1
70.00 KB, patch
Details | Diff | Splinter Review
Part 2. Improve reuse of and releasing of device storage objects where appropriate. -- v6
27.18 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Whiteboard: [NG Device Storage]
Moves preferences and observers into a dedicated module which is much easier to make multithreaded when the time comes.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b305a945a071
In Navigator, we keep a list of device storage objects. Some components constantly cause new device storage objects to be created even though they could (in theory) reuse their existing instance. These objects do not get freed until the window is shutdown, so they may have a very long lifetime. This is problematic due to memory pressures and an ever increasing number of stale objects we need to maintain and send notifications too.

Instead search the list of existing objects for a match on type/name/principal and return that if found. Otherwise create a new instance. Also make references weak so that the garbage collector can actually cleanup the old forgotten-by-JS objects.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2e661b11c42
Component: DOM: Core & HTML → DOM: Device Interfaces
Turn off logging by default.
Attachment #8637203 - Attachment is obsolete: true
Attachment #8637233 - Flags: review?(dhylands)
Cleanup.
Attachment #8637204 - Attachment is obsolete: true
Attachment #8637234 - Flags: review?(dhylands)
Comment on attachment 8637234 [details] [diff] [review]
Part 2. Improve reuse of and releasing of device storage objects where appropriate. -- v3.1

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

Some of my own comments.

::: dom/base/Navigator.cpp
@@ -206,5 @@
>  #endif
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCameraManager)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mMediaDevices)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mMessagesManager)
> -  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mDeviceStorageStores)

I don't *think* weak pointers need to be in cycle collection, as they don't create a cycle. But I couldn't find any confirmation of that.

@@ +1017,5 @@
> +      if (storage) {
> +        aStores.AppendElement(storage.forget());
> +      }
> +    }
> +  }

This allows us to reuse the main process creating 'sdcard', 'sdcard1' and 'usbdisk' for the type 'sdcard'  every time the screen is powered on and off. That was making the observer lists build up in a hurry!

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +3204,5 @@
> +
> +  nsCOMPtr<nsPIDOMWindow> window = GetOwner();
> +  return aWin && aWin == window &&
> +         mStorageName.Equals(aName) &&
> +         mStorageType.Equals(aType);

Can the principal for a window change? Originally I was checking that the principal was the same, and that seems to work equivalently (for testing purposes) so I can enforce both match if preferred, as we probably don't want to permit sharing in the case the principals match but the windows / parent objects are different.

Going forward for workers, we may be able to do a limited principal check and window ID check. It might not permit reusing in as many cases but I don't think that is a huge problem (just more churn on the objects, at least they still get cleared out with the weak references).
Comment on attachment 8637233 [details] [diff] [review]
Part 1. Move preferences and observers into dedicated threadsafe module. -- v3.1

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

Looks great. I should be able to review part 2 tomorrow.

::: dom/devicestorage/DeviceStorageStatics.cpp
@@ +89,5 @@
> +
> +void
> +DeviceStorageStatics::InitDirs()
> +{
> +  DS_LOG_INFO("");

Along the lines of better logging, I think that it would be really useful to log each of the directories for each storage type once at startup.

This can probably be done in InitProfileDirs (or split between the 2).

For gonk, we'd report the mount point each time the volume transitions into the MOUNTED state.

@@ +220,5 @@
> +  Preferences::RemoveObserver(this, kPrefWritableName);
> +}
> +
> +/* static */ already_AddRefed<nsIFile>
> +DeviceStorageStatics::GetDir(DeviceStorageDir aName)

Let's use aType or aStorageType rather than aName (see other comment about DeviceStorageDir)

@@ +240,5 @@
> +    default:
> +      break;
> +  }
> +
> +  if (!file) {

Won't this always be true?

@@ +424,5 @@
> +void
> +DeviceStorageStatics::ResetOverrideRootDir()
> +{
> +  nsCOMPtr<nsIFile> f;
> +

I think we should assert that we're on the main thread here (I'm getting more and more paranoid as we get more and more threading stuff added :)

@@ +453,5 @@
> +    if (XRE_IsParentProcess()) {
> +      // Only the parent process can create directories. In testing, because
> +      // the preference is updated after startup, its entirely possible that
> +      // the preference updated notification will be received by a child
> +      // prior to the parent.

Hmm. I think that could cause a problem.

If the child can receive the preference notification before the parent, then this means that the child can also go ahead and try to put files into the non-existant directory before the parent gets around to creating the directory.

So I think that this could lead to test failing mysteriously.

We probably need to add a "fix" to the tests where it waits for the the directory to exist after setting the preference (or maybe the tests already create the directory, and this is to cover another use-case, like making the directory as a convenience for the desktop user).

@@ +481,5 @@
> +    StaticMutexAutoLock lock(sMutex);
> +    nsDependentString name(aData);
> +    if (name.EqualsASCII(kPrefTesting) ||
> +        name.EqualsASCII(kPrefOverrideRootDir))
> +    {

nit: open curly should be at end of previous line

@@ +524,5 @@
> +      return NS_OK;
> +    }
> +
> +    StaticMutexAutoLock lock(sMutex);
> +    auto data = NS_ConvertUTF16toUTF8(aData);

Heh - I've not seen this before (the use of auto like this). Learn something new every day...

@@ +577,5 @@
> +     from one child to another child in B2G.  (f.e., if one child decides to
> +     add a file, we want to be able to able to send a onchange notifications
> +     to every other child watching that device storage object). */
> +  nsRefPtr<DeviceStorageFile> dsf;
> +  if (!strcmp(aTopic, kDownloadWatcherNotify)) {

The comment says file-watcher-notify, but the constant is download-watcher-notify

::: dom/devicestorage/DeviceStorageStatics.h
@@ +41,5 @@
> +  static already_AddRefed<nsIFile> GetMusicDir();
> +  static already_AddRefed<nsIFile> GetSdcardDir();
> +
> +private:
> +  enum DeviceStorageDir {

I've been referring to these as StorageTypes, so perhaps DeviceStorageType makes more sense?

::: dom/devicestorage/nsDeviceStorage.h
@@ +38,5 @@
>  class Blob;
>  } // namespace dom
>  } // namespace mozilla
>  
> +#if 0

This can go in a separate bug, but I'd really like to see the ability to turn this on or off via preference and/or a setting in the settings app.
Attachment #8637233 - Flags: review?(dhylands) → review+
Comment on attachment 8637234 [details] [diff] [review]
Part 2. Improve reuse of and releasing of device storage objects where appropriate. -- v3.1

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

Looks good - The only real issue I saw was the implementation of GetDeviceStorage seems incorrect, so r+ is contingent on fixing that.

You can play with this using DS Test (present in eng builds) and use GetDeviceStorage and GetDeviceStorages, and then change the default in the settings app (under Media Storage at the very bottom is the "Default Media Location")

::: dom/base/Navigator.cpp
@@ +969,5 @@
>      return nullptr;
>    }
>  
> +  nsString name;
> +  nsRefPtr<nsDOMDeviceStorage> storage = FindDeviceStorage(name, aType);

This doesn't look right to me.

GetDeviceStorages should return all of the storage areas. GetDeviceStorage should return the "default" storage area.

CreateDeviceStorageFor does:
https://dxr.mozilla.org/mozilla-central/source/dom/devicestorage/nsDeviceStorage.cpp?from=CreateDeviceStorageFor#3556-3562

I think we can put the IsVolumeBased check inside GetDefaultStorageName, and then we should initialize name above with the default storage name returned from GetDefaultStorageName.

::: dom/base/Navigator.h
@@ +387,5 @@
>  #endif
>    nsRefPtr<nsDOMCameraManager> mCameraManager;
>    nsRefPtr<MediaDevices> mMediaDevices;
>    nsCOMPtr<nsIDOMNavigatorSystemMessages> mMessagesManager;
> +  nsTArray<nsWeakPtr > mDeviceStorageStores;

nit: no space before >

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +3091,5 @@
>  // static
>  void
>  nsDOMDeviceStorage::GetOrderedVolumeNames(
> +  const nsAString& aType,
> +  nsDOMDeviceStorage::VolumeNameArray &aVolumeNames)

nit: move &

@@ +3093,5 @@
>  nsDOMDeviceStorage::GetOrderedVolumeNames(
> +  const nsAString& aType,
> +  nsDOMDeviceStorage::VolumeNameArray &aVolumeNames)
> +{
> +  if (!DeviceStorageTypeChecker::IsVolumeBased(aType)) {

I think that this sould so aVolumeNames.Truncate() just to be sure its returning an empty list.

@@ +3106,1 @@
>    nsDOMDeviceStorage::VolumeNameArray &aVolumeNames)

nit:move & (not your fault - oh well)

@@ +3204,5 @@
> +
> +  nsCOMPtr<nsPIDOMWindow> window = GetOwner();
> +  return aWin && aWin == window &&
> +         mStorageName.Equals(aName) &&
> +         mStorageType.Equals(aType);

I think checking for window should be fine.
Attachment #8637234 - Flags: review?(dhylands) → review+
You should also look at bug 1186750 (especially Part 8).

I seem to recall that you added some calls to NS_DispatchXxx and we should probably be consistent.
Blocks: 1189004
Incorporate review feedback. Fix Android and emulator test failures; apparently there was still a timing issue for when to populate the directory paths. I've reverted to initializing it when InitDirs() was originally called, with the anticipation we would also need to call it when a worker is spawned (also main thread) in the future.

> @@ +453,5 @@
> > +    if (XRE_IsParentProcess()) {
> > +      // Only the parent process can create directories. In testing, because
> > +      // the preference is updated after startup, its entirely possible that
> > +      // the preference updated notification will be received by a child
> > +      // prior to the parent.
> 
> Hmm. I think that could cause a problem.
> 
> If the child can receive the preference notification before the parent, then
> this means that the child can also go ahead and try to put files into the
> non-existant directory before the parent gets around to creating the
> directory.
> 
> So I think that this could lead to test failing mysteriously.
> 
> We probably need to add a "fix" to the tests where it waits for the the
> directory to exist after setting the preference (or maybe the tests already
> create the directory, and this is to cover another use-case, like making the
> directory as a convenience for the desktop user).

Hm, I see your concern (at least in theory) but this should be fixed in another change as I just moved the existing handler code but did not modify it.

In practice, I think it is okay though. If the child process wants to use the directory in any meaningful way, it will still need to go through the parent process / content IPC. Since it processes the IPC calls in order, the directory creation will get processed first.
Attachment #8637233 - Attachment is obsolete: true
Incorporate review feedback, including bug 1186750. Simplify listener management by moving the weak reference to device storage instead of DeviceStorageStatics.

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

*fingers crossed*
Attachment #8637234 - Attachment is obsolete: true
Fixes some static build issues.
Attachment #8641069 - Attachment is obsolete: true
try (full): https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b718ce19474
try (after statics fix): https://treeherder.mozilla.org/#/jobs?repo=try&revision=41b3038d88f4

There appears to be an intermittent timeout for dom/devicestorage/test/test_fs_app_permissions.html which warrants further investigation. There is already an existing bug opened for this but I wonder if I made it more reproducible...

Other than that, the test results look good.
Attachment #8641070 - Attachment is obsolete: true
See bug 1020794 for historical / intermittent test_fs_app_permissions.html failures.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=881fa09c9886

Switching to weak references caused a potential race condition where the device storage object may be cycle collected before a DeviceStorage::getRoot operation completes. The freeing caused nsDOMDeviceStorage::Shutdown and DeviceStorageFileSystem::Shutdown to be called. The latter shutdown causes it to drop the operation instead of failing (presumably because in a shutdown scenario, there is nothing to listen on the promise result!).

Change nsDOMDeviceStorage to simply deregister with the statics module in the destructor, but not call shutdown on the file system. The file system modules appear to handle this gracefully.
Attachment #8641933 - Attachment is obsolete: true
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11c5493ca671

Updated to avoid a race condition on device storage getting released prior to the completion of the dom/filesystem request. Now instead of having a device storage weak reference, it simply has the window ID, which it converts into a pointer on demand. The parent object of the promise for the file system request is the window, which should keep the window alive (and thus the ID valid) for the duration.
Attachment #8648869 - Attachment is obsolete: true
Attachment #8649056 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b855bb08a22b
https://hg.mozilla.org/mozilla-central/rev/434643c56a28
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: