When files are added/removed from Device Storage, this should also be reflected in MTP, and vice-versa.

Eric - this version doesn't run properly. It crashes somewhere. It was running until I changed some of the Mutex stuff. I'm pretty confident that the problem will be something dumb, so I thought I'd put it up for review in its present form. Obviously, I can't land this until I figure out what's going on, but I figured I'd better get the review process rolling.

Bah - Sorry - I forogt to qref.

This version works. I didn't obsolete the old patch in case you were reviewing it.

Here's the interdiff between v2 and v3: https://bugzilla.mozilla.org/attachment.cgi?oldid=8479588&action=interdiff&newid=8479610&headers=1

(In reply to Dave Hylands [:dhylands] from comment #4) > Here's the interdiff between v2 and v3: > https://bugzilla.mozilla.org/attachment. > cgi?oldid=8479588&action=interdiff&newid=8479610&headers=1 Thanks! I'll review v3 today.

Hi Eric, I'm tracking this feature for landing in 2.1. Can you provide an update on the ETA for your review?

Found a deadlock path. Removed an extraneous mutex lock. Cleaned up some of the logging.

Comment on attachment 8480238 [details] [diff] [review] Pt 2. Integrate MTP & Device Storage I discovered one more needed change while testing today. So I'm atatching it as Part 2 (since you're probably part way through your review).

(In reply to Mike Habicher [:mikeh] from comment #6) > Hi Eric, I'm tracking this feature for landing in 2.1. Can you provide an > update on the ETA for your review? I have already started my review from yesterday and will get it done today.

(In reply to Dave Hylands [:dhylands] from comment #8) > Comment on attachment 8480238 [details] [diff] [review] > Pt 2. Integrate MTP & Device Storage > > I discovered one more needed change while testing today. > > So I'm atatching it as Part 2 (since you're probably part way through your > review). Yes, I am. Thanks!

Comment on attachment 8479610 [details] [diff] [review] Integrate MTP & Device Storage v3 Review of attachment 8479610 [details] [diff] [review]: ----------------------------------------------------------------- ok, overall looks good to me. Only a few nits and questions need to be addressed. In addition, please merge the two patches that we already have into one after modification. Thanks! ::: dom/system/gonk/MozMtpDatabase.cpp @@ +112,5 @@ > entry->mHandle, entry->mParent, entry->mPath.get()); > } > > +void > +MozMtpDatabase::DumpEntries(const char* label) nit: "aLabel" @@ +187,5 @@ > + > + NS_ConvertUTF8toUTF16 storageName(mStorageName); > + NS_ConvertUTF8toUTF16 path(mPath); > + > + nsRefPtr<DeviceStorageFile> dsf(new DeviceStorageFile(NS_LITERAL_STRING(DEVICESTORAGE_SDCARD), nit: 80-chars per line, please. @@ +201,5 @@ > + return NS_OK; > + } > + > +private: > + nsAutoCString mStorageName; We should not use nsAuto*String for member variables since it pre-allocates too much memory on stack. (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings#Member_variables) @@ +206,5 @@ > + nsAutoCString mPath; > + nsAutoCString mEventType; > +}; > + > + nit: extra blank line. @@ +212,5 @@ > +// through the MTP server. > +void > +MozMtpDatabase::FileWatcherNotify(DbEntry* aEntry, const char* aEventType) > +{ > + // This function gets called from the MozMtpServer::mServerThread Please add an assertion to check the thread it gets called on. @@ +225,5 @@ > + } > + RefPtr<StorageEntry> storageEntry; > + { > + MutexAutoLock lock(mMutex); > + storageEntry = mStorage[storageIndex]; This lock doesn't seem to live long enough. It only protects the assignment but not other operations of storageEntry below. Is that correct? (and please just correct me if I'm totally wrong with it :) ) @@ +340,5 @@ > + > + bool exists = false; > + aFile->mFile->Exists(&exists); > + if (!exists) { > + // File doesn't exist, no sense telling MTP about it. This case is pretty weird to me though. Can we add a MTP_DBG() here? @@ +353,5 @@ > + int32_t slash; > + > + do { > + nsDependentCSubstring component; > + nsCString dbgPath2(aPath); Unused variable. Please remove it. @@ +403,5 @@ > + entry->mDateModified = entry->mDateCreated; > + > + AddEntry(entry); > + parent = entry->mHandle; > + } while (slash != kNotFound); To be honest, the logic inside this do-while loop looks a little complicated to me. I would like to know if it's possible to improve the readability such as break this loop into two (maybe take the line |doFind = false| as a separator). Do you think this would work? @@ +487,5 @@ > + for (storageIndex = 0; storageIndex < numStorages; storageIndex++) { > + RefPtr<StorageEntry> storage = mStorage[storageIndex]; > + if (StringHead(aPath, storage->mStoragePath.Length()).Equals(storage->mStoragePath)) { > + if (aPath.Length() == storage->mStoragePath.Length()) { > + return storage->mStorageID; Question: do we need to truncate aRemainder in this case? @@ +498,5 @@ > + } > + return 0; > +} > + > + super-nit: extra blank line @@ +504,5 @@ > MozMtpDatabase::AddStorage(MtpStorageID aStorageID, > const char* aPath, > const char* aName) > { > + super-nit: extra blank line @@ +565,5 @@ > MtpObjectFormat aFormat, > MtpObjectHandle aParent, > MtpStorageID aStorageID, > uint64_t aSize, > time_t aModified) Please fix the alignment. @@ +650,5 @@ > for (entryIndex = 1; entryIndex < numEntries; entryIndex++) { > RefPtr<DbEntry> entry = mDb[entryIndex]; > + if (entry && > + (aStorageID == 0xFFFFFFFF || entry->mStorageID == aStorageID) && > + (aParent == 0 || entry->mParent == aParent)) { Good catch. After applying this fix, we still don't check aFormat here. Do you think it's worth filing a follow-up or adding a TODO comment? @@ +690,5 @@ > for (entryIndex = 1; entryIndex < numEntries; entryIndex++) { > RefPtr<DbEntry> entry = mDb[entryIndex]; > + if (entry && > + (aStorageID == 0xFFFFFFFF || entry->mStorageID == aStorageID) && > + (aParent == 0 || entry->mParent == aParent)) { Same comment as above. ::: dom/system/gonk/MozMtpDatabase.h @@ +226,5 @@ > MatchParentFormat, > }; > > + void AddEntry(DbEntry* aEntry); > + void DumpEntries(const char* label); nit: "aLabel" ::: dom/system/gonk/MozMtpServer.cpp @@ +54,5 @@ > + {} > + > + NS_IMETHOD Run() > + { > + // Runs on the FileWatcherUpdate->mIOThread Please add an assertion MOZ_ASSERT(!NS_IsMainThread()) to assure this runnable running on non-main thread. @@ +73,5 @@ > +{ > +public: > + NS_DECL_THREADSAFE_ISUPPORTS > + > + FileWatcherUpdate(MozMtpServer* mozMtpServer) nit: "aMozMtpServer" @@ +134,5 @@ > + nsRefPtr<MozMtpServer> mMozMtpServer; > + nsCOMPtr<nsIThread> mIOThread; > +}; > +NS_IMPL_ISUPPORTS(FileWatcherUpdate, nsIObserver) > +static mozilla::StaticRefPtr<FileWatcherUpdate> sFileWatcherUpdate; not-even-a-nit: the prefix "mozilla::" is not required since namespace "mozilla" has been in use. @@ +163,5 @@ > + {} > + > + NS_IMETHOD Run() > + { > + MOZ_ASSERT(NS_IsMainThread()); Would you mind inserting a blank line here after the assertion?

(In reply to Eric Chou [:ericchou] [:echou] from comment #11) > Comment on attachment 8479610 [details] [diff] [review] > Integrate MTP & Device Storage v3 > > Review of attachment 8479610 [details] [diff] [review]: > @@ +212,5 @@ > > +// through the MTP server. > > +void > > +MozMtpDatabase::FileWatcherNotify(DbEntry* aEntry, const char* aEventType) > > +{ > > + // This function gets called from the MozMtpServer::mServerThread > > Please add an assertion to check the thread it gets called on. Right now, there isn't any convenient way to do that. The thread its running on is MozMtpServer::mServerThread and the database doesn't have access to that. I'll add a !Main thread assert So I added the comment instead. > @@ +225,5 @@ > > + } > > + RefPtr<StorageEntry> storageEntry; > > + { > > + MutexAutoLock lock(mMutex); > > + storageEntry = mStorage[storageIndex]; > > This lock doesn't seem to live long enough. It only protects the assignment > but not other operations of storageEntry below. Is that correct? (and please > just correct me if I'm totally wrong with it :) ) The mutex protects the array. We need to protect against the array being reallocated by another thread. If that happens the object that gets returned points into free memory. In our case, the object being returned is a thread-safe ref-counted object. So our reference to the object remains valid even after the mutex. If another thread resizes the array, that's fine, it will wind up incrementing and decrementing the ref count. The storage objects in the array themselves don't change. Once they're added they don't change until they get destroyed. So in this case, we don't need any additional protection. > @@ +340,5 @@ > > + > > + bool exists = false; > > + aFile->mFile->Exists(&exists); > > + if (!exists) { > > + // File doesn't exist, no sense telling MTP about it. > > This case is pretty weird to me though. Can we add a MTP_DBG() here? I think that this would happen if device storage created and removed a file. Since the notifications are async, by the time we get around to processing them, the file would already be gone. I'll add a comment. I don't think a DBG is warranted. > @@ +403,5 @@ > > + entry->mDateModified = entry->mDateCreated; > > + > > + AddEntry(entry); > > + parent = entry->mHandle; > > + } while (slash != kNotFound); > > To be honest, the logic inside this do-while loop looks a little complicated > to me. I would like to know if it's possible to improve the readability such > as break this loop into two (maybe take the line |doFind = false| as a > separator). Do you think this would work?\ I think I'll file a bug to clean this up. To clean it up, I think what we want is a tokenizer that returns the next component of a path. Then you'd wind up with 2 loops. The first loop calls the tokenizer (which is essentially the first part of the loop) and the stuff inside doFind. Then when you hit a directory which doesn't exist, you drop out of the first loop and into the second loop (which then has to create directory entries and the final file entry) The doFind boolean was just an optimization to not bother calling FindEntryByPath on entires we know don't exist. > @@ +487,5 @@ > > + for (storageIndex = 0; storageIndex < numStorages; storageIndex++) { > > + RefPtr<StorageEntry> storage = mStorage[storageIndex]; > > + if (StringHead(aPath, storage->mStoragePath.Length()).Equals(storage->mStoragePath)) { > > + if (aPath.Length() == storage->mStoragePath.Length()) { > > + return storage->mStorageID; > > Question: do we need to truncate aRemainder in this case? I like to initialize output parameters to a known state, so that on an error path we're not returning uninitialized data. It's needed for the success case when you pass in a root directory. I also wanted it in the error case (so paranoid protection if the caller passed in something in aRemainder) If the caller passes in an empty string, then the Truncate is essentially a no-op. > @@ +650,5 @@ > > for (entryIndex = 1; entryIndex < numEntries; entryIndex++) { > > RefPtr<DbEntry> entry = mDb[entryIndex]; > > + if (entry && > > + (aStorageID == 0xFFFFFFFF || entry->mStorageID == aStorageID) && > > + (aParent == 0 || entry->mParent == aParent)) { > > Good catch. After applying this fix, we still don't check aFormat here. Do > you think it's worth filing a follow-up or adding a TODO comment? I'll add the Format check. I don't think it really gets used for file-only operations, but it should be there anyways (in our case the format distinguishes files from directories).

https://hg.mozilla.org/integration/b2g-inbound/rev/812f62c9f703