931224 - [Settings] Blobs/Files should be retrieved from the DB via get() after their put() to make them IndexedDB-backed Files before notifying settings observers

Status: RESOLVED INCOMPLETE

(Core :: DOM: Device Interfaces, defect)

Description

[David Flanagan [:djf]]

Okay, this bug is a little hard to explain. For context, this is about FirefoxOS. The settings app uses a web activity to invoke the ringtones app. The user chooses a ringtone or alert tone, and the ringtones app returns the sound file as a blob to the settings app, which stores it in the settings db.  

I filed bug 914404 because I was seeing a crash when I saved a blob to the settings db.  That bug has since gone away and I can't reproduce it anymore, so I'll be closing the bug. 

While the bug was reproducible, however, my workaround was to save the blob as a file (using device storage) then return the filename (via web activities) then read the file (using device storage) and save that file to the settings database, and then delete the file.

This workaround prevented the crash for me.  But it turns out that it caused bug 923544. That bug was that after changing the notification sound, the notification sound would not play until the phone was rebooted.  After a reboot it worked fine. Ringtones worked fine. But notification tones did not. Ringtones are played by the dialer app which runs OOP (I think).  Notification tones are played by the system app, so they are in process.

I'm guessing that the problem was that there was some kind of race condition where saving the file into the settings db caused notifications that the settings had changed, and the system app created a new blob: url for the new setting value. Then the file got deleted in device storage, but somehow there was a cache that was relying on the existance of the file.  The data did get copied into the settings db because after reboot, the sound played just fine.  But before reboot, I had a valid blob url, but no sound would play.  (No errors from the <audio> element either).

IIRC there are different paths for in process and out of process blob stuff, so maybe there is some kind of overly aggressive caching going on in the in-process path?

I hope this makes sense.  I'm going to be backing out my workaround to bug 914404, so I don't have a test case that you can try this out with, but I did want to file it.

I'm guessing that this is a blob bug for Ben. But I'm also setting needinfo on Dave and Gregor since it could involve DeviceStorage or SettingsDB.

Comment 1

[Dave Hylands [:dhylands]]

So based on the description, this sounds more like a problem with the settings db, rather than device storage.

One subtlety of linux when using an ext4 based file system (I don't think that this applies to VFAT), is that if you have a file open and then delete it, the open file handle remains valid even though the file has been deleted. Note that the file hasn't actually been deleted from disk, just the directory entry. When the open file handle count goes to zero, then the file would actually be removed from disk.

I believe that all of the volumes served up by device storage are VFAT based, except on the nexus 4. With VFAT, I'm not sure what the exact behaviour is.

But anyways, in the original manifestation of the bug, it sounds like you weren't using devices storage at all. And to the best of my knowledge device storage doesn't do any caching of any files - just free space and available space.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'm not sure what to recommend really, this sounds very odd. We don't really have any blob caches that I'm aware of, and IndexedDB seems to have successfully copied the file...

I wonder if the media cache might be involved? I have no idea how the notification sounds are played, but if they go through an audio element maybe we're holding onto those longer or something?

Comment 3

[Gregor Wagner [:gwagner]]

Hm can you check if this does the right thing for you?
https://github.com/mozilla-b2g/gaia/blame/master/apps/system/js/notifications.js#L92

Is there an easy way to reproduce this?

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

I was just looking into v1.4 (and v1.5?) bug 989570 where a lock-screen background picture disappears, apparently because the memory-backed Blob death problem still exists (bug 982779 is where I have most recently complained about it.)

I think that bug and this bug are effectively the same thing.  If you look in SettingsManager's "set" implementation:
http://dxr.mozilla.org/mozilla-central/source/dom/settings/SettingsManager.js?from=SettingsManager.js&case=true#87


It does a .put(obj):
  let obj = {settingName: key, defaultValue: defaultValue, userValue: userValue};
  let setReq = store.put(obj);
which will include the Blob/File and then in onsuccess:
  cpmm.sendAsyncMessage("Settings:Changed", { key: key, value: userValue });
which notifies the observer.

But since there is no .get() in that onsuccess, we are sending out the exact same Blob/File that came to us.  So in the comment 0 case, that will be the DeviceStorage-backed File which will now have been deleted.  (In contrast to a high-quality IndexedDB-backed File which will be referenced counted under the hood.  Note that I'm trying to curry favor with the IndexedDB people with that, not be sarcastic.)

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

FxOS/Gonk has been removed from the codebase. Mass-invalidating FxOS related Device Interface bugs to clean up the component. 

If I incorrectly invalidated something, please let me know.

Comment 6

[Firefox Bug Husbandry Bot]

Bulk correction of resolution of B2G bugs to INCOMPLETE.