Integrate MTP into Device Storage

RESOLVED FIXED in 2.1 S3 (29aug)

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dhylands, Assigned: dhylands)

Tracking

unspecified
2.1 S3 (29aug)
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.1)

Details

(Whiteboard: [ft:devices])

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

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

Updated

5 years ago
Target Milestone: --- → 2.1 S3 (29aug)

Updated

5 years ago
Assignee: nobody → dhylands
feature-b2g: --- → 2.1
Whiteboard: [ft:devices]
Assignee

Comment 1

5 years ago
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.
Attachment #8479586 - Flags: review?(echou)
Assignee

Comment 2

5 years ago
Bah - Sorry - I forogt to qref.
Attachment #8479586 - Attachment is obsolete: true
Attachment #8479586 - Flags: review?(echou)
Attachment #8479588 - Flags: review?(echou)
Assignee

Comment 3

5 years ago
This version works. I didn't obsolete the old patch in case you were reviewing it.
Attachment #8479610 - Flags: review?(echou)
(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.
Assignee

Updated

5 years ago
Attachment #8479588 - Attachment is obsolete: true
Attachment #8479588 - Flags: review?(echou)
Hi Eric, I'm tracking this feature for landing in 2.1. Can you provide an update on the ETA for your review?
Flags: needinfo?(echou)
Assignee

Comment 7

5 years ago
Found a deadlock path. Removed an extraneous mutex lock.

Cleaned up some of the logging.
Assignee

Comment 8

5 years ago
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).
Attachment #8480238 - Flags: review?(echou)
(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.
Flags: needinfo?(echou)
(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?
Attachment #8479610 - Flags: review?(echou) → review+
Attachment #8480238 - Flags: review?(echou) → review+
Assignee

Comment 12

5 years ago
(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).
Assignee

Updated

5 years ago
See Also: → 1059880
Assignee

Updated

5 years ago
See Also: → 1059898
https://hg.mozilla.org/mozilla-central/rev/812f62c9f703
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.