793955 - [DeviceStorage] files returned by DeviceStorage.get() don't always have lastModifiedDate

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[David Flanagan [:djf]]

When the Gaia camera app saves a new photo to device storage, the Gaia gallery app gets a change event with its reason property set to 'modified' and its path property set to the filename of the new file.

When the gallery app calls get() on that filename to get the File object, the result does not have a lastModifiedDate property (perhaps it is a Blob rather than a true File object?).  

Gallery wants to display photos from most recent to least recent, so having file modification times is important.  As a workaround, I can use Date.now() as the timestamp for any file that we get a change event about.  But this isn't foolproof. If the gallery app exits and is restarted, then mediadb will rescan the /sdcard (using DeviceStorage.enumerate()) when the app starts up again.  When it does this, the new photo file has a valid lastModifiedDate, but that date won't match the fake value I used for the bug workaround.  And this means that mediadb will think that the file has changed since it was created. It will treat it as if it had been deleted and then created again. And this will cause its metadata to be reparsed, and the Gallery UI to flash.

I'm assuming that this only happens when calling get() shortly after a change/modified event arrives. But I haven't verified that... Maybe get() always returns a Blob with no lastModifiedDate instead of a true File object.

I've got a partial, sub-optimal workaround, so I'm not sure that this blocks, but I'm going to nominate it.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 2

[Andrew Overholt [:overholt]]

Can this make feature freeze?  Seems like not having this could quite negatively affect gallery usage.

Comment 3

[Doug Turner (:dougt)]

just needs to review.  it's a pretty patch.

Comment 4

[Doug Turner (:dougt)]

khuey ^

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 664547 [details] [diff] [review]
patch v.1

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

I'm surprised this patch changes anything.  nsContentUtils::WrapNative should look at the classinfo, not just the IID you passed in, and wrap the object to be a File if appropriate, no?

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 664547 [details] [diff] [review]
patch v.1

we also aren't remoting the lastmodificationdate now...

Comment 7

[Doug Turner (:dougt)]

Attachment: patch v.1

bent, i am not sure this is correct -- if adding a new noscript attribute is the way to go.

Comment 8

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

Comment on attachment 667289 [details] [diff] [review]
patch v.1

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

::: dom/ipc/Blob.cpp
@@ +371,5 @@
>    typedef InputStreamActor<ActorFlavor> StreamActorType;
>  
>  private:
>    ActorType* mActor;
> +  PRTime mModDate;

We've removed support for a bunch of old PR-types and I'd rather not add more, can we use TimeStamp here?

::: dom/ipc/ContentChild.cpp
@@ +521,5 @@
> +      file->GetMozFileInternal(getter_AddRefs(rawFile));
> +
> +      PRTime msecs = 0;
> +      if (rawFile) {
> +        rawFile->GetLastModifiedTime(&msecs);

This adds main thread IO, gotta figure out some other way.

Comment 9

[Doug Turner (:dougt)]

> We've removed support for a bunch of old PR-types and I'd rather not add more, can we use TimeStamp here?

the nsIFile interface still uses NSPR types.



> This adds main thread IO, gotta figure out some other way.

Right now, if anyone calls blob.lastModifiedDate or blob.size, that causes a stat():

  http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDOMFile.cpp#503
  http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDOMFile.cpp#522

Right above my call to GetLastModificatedTime, we are stat'ing the file for its size:

  http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#1225

Comment 10

[Doug Turner (:dougt)]

Attachment: patch v.2

this is getting a bit closer.  Ben, can you take a look?

Comment 11

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

Comment on attachment 669683 [details] [diff] [review]
patch v.2

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

This is really close, yeah!

::: content/base/public/nsDOMFile.h
@@ +53,5 @@
>    NS_DECL_NSIMUTABLE
>  
>    void
>    SetLazyData(const nsAString& aName, const nsAString& aContentType,
> +              uint64_t aLength, uint64_t aLastModifiedDate = INT64_MAX)

This should be UINT64_MAX, right?

But actually why are we allowing an unspecified date here? The purpose of this method is to fill in all the blanks.

@@ +71,5 @@
>    }
>  
> +  bool IsDateUnknown() const
> +  {
> +    return mLastModificationDate == INT64_MAX;

UINT64_MAX

@@ +157,5 @@
>  class nsDOMFile : public nsDOMFileBase
>  {
>  public:
>    nsDOMFile(const nsAString& aName, const nsAString& aContentType,
> +            uint64_t aLength, uint64_t aLastModifiedDate = UINT64_MAX)

Same here, I think this version should not have an optional date.

@@ +218,5 @@
>      NS_ASSERTION(mFile, "must have file");
>    }
>  
> +  nsDOMFileFile(const nsAString& aName, const nsAString& aContentType,
> +                uint64_t aLength, nsIFile *aFile, PRTime aLastModificationDate)

Switching to take a PRTime here is a little weird. Maybe the DOM stuff should only ever deal with uint64_t?

@@ +289,5 @@
>    // Overrides
>    NS_IMETHOD GetSize(uint64_t* aSize);
>    NS_IMETHOD GetType(nsAString& aType);
>    NS_IMETHOD GetLastModifiedDate(JSContext* cx, JS::Value *aLastModifiedDate);
> +  NS_IMETHOD GetMozLastModifiedDate(uint64_t *aLastModifiedDate);

Nit: Both those date functions should have the * on the left of the space.

::: content/base/public/nsIDOMFile.idl
@@ +72,5 @@
>    readonly attribute DOMString mozFullPath;
>  
>    // This performs no security checks!
>    [noscript] readonly attribute DOMString mozFullPathInternal;
> +  [noscript] readonly attribute uint64_t mozLastModifiedDate;

Nit: I'd add a newline here or move above mozFullPathInternal so that someone doesn't think the security warning comment applies to the date getter.

::: content/base/src/nsDOMFile.cpp
@@ +506,5 @@
>    return mFile->GetPath(aFilename);
>  }
>  
>  NS_IMETHODIMP
>  nsDOMFileFile::GetLastModifiedDate(JSContext* cx, JS::Value *aLastModifiedDate)

Doesn't look like this asserts that it's only called on files, probably should.

::: dom/ipc/Blob.cpp
@@ +660,5 @@
>      return static_cast<typename ActorType::ProtocolType*>(mActor);
>    }
> +
> +    NS_IMETHOD
> +    GetLastModifiedDate(JSContext* cx, JS::Value *aLastModifiedDate)

Nit: You're using 4-space indent here, should be 2.

@@ +663,5 @@
> +    NS_IMETHOD
> +    GetLastModifiedDate(JSContext* cx, JS::Value *aLastModifiedDate)
> +    {
> +        aLastModifiedDate->setNull();
> +        if (!IsDateUnknown()) {

Nit: I prefer only setting once, so:

  if (IsDateUnknown()) {
    aLastModifiedDate->setNull();
  }
  else {
    // ...
  }

@@ +665,5 @@
> +    {
> +        aLastModifiedDate->setNull();
> +        if (!IsDateUnknown()) {
> +            JSObject* date = JS_NewDateObjectMsec(cx, mLastModificationDate);
> +            if (date) {

If this fails you should return NS_ERROR_OUT_OF_MEMORY.

@@ +805,2 @@
>  
> +  ToConcreteBlob(mBlob)->SetLazyData(aName, aContentType, aLength, aLastModifiedDate);

Nit: Please check 80 char limit here.

@@ +807,2 @@
>  
> +  FileBlobConstructorParams params(aName, aContentType, aLength, aLastModifiedDate);

And here.

@@ +821,5 @@
>  
>    nsString voidString;
>    voidString.SetIsVoid(true);
>  
> +  ToConcreteBlob(mBlob)->SetLazyData(voidString, aContentType, aLength, UINT64_MAX);

And here.

@@ +922,5 @@
>  
>      case ResolveMysteryParams::TFileBlobConstructorParams: {
>        const FileBlobConstructorParams& params =
>          aParams.get_FileBlobConstructorParams();
> +      blob->SetLazyData(params.name(), params.contentType(), params.length(), params.modDate());

And here.

::: dom/ipc/Blob.h
@@ +172,5 @@
>  
>    // Use this for files.
>    bool
>    SetMysteryBlobInfo(const nsString& aName, const nsString& aContentType,
> +                     uint64_t aLength, uint64_t aLastModifiedDate = UINT64_MAX);

Nit: I think this goes past 80 chars, please wrap.

::: dom/ipc/ContentChild.cpp
@@ +546,5 @@
>        rv = file->GetName(fileParams.name());
>        NS_ENSURE_SUCCESS(rv, nullptr);
>  
> +      uint64_t modDate;
> +      rv = file->GetMozLastModifiedDate(&modDate);

Nit: See comment in ContentParent.cpp

::: dom/ipc/ContentParent.cpp
@@ +1237,5 @@
>      if (file) {
>        FileBlobConstructorParams fileParams;
>  
> +      uint64_t modDate;
> +      rv = file->GetMozLastModifiedDate(&modDate);

Nit: just use |fileParams.modDate()| here and lose the temporary.

Comment 12

[Alex Keybl [:akeybl]]

(In reply to David Flanagan [:djf] from comment #0)
> When the Gaia camera app saves a new photo to device storage, the Gaia
> gallery app gets a change event with its reason property set to 'modified'
> and its path property set to the filename of the new file.

Please block this bug on the bug tracking the Gallery change.

Comment 13

[Alex Keybl [:akeybl]]

We've also heard the Gallery app doesn't appear to be affected by this for pictures taken from the Camera anymore - would be good to confirm. They may be relying on filename right now.

Comment 14

[David Flanagan [:djf]]

(In reply to Alex Keybl [:akeybl] from comment #12)
> 
> Please block this bug on the bug tracking the Gallery change.

I've got a workaround (even if it isn't a very good one).  I don't think I ever filed a bug specific to gallery for this. When this bug is fixed the gallery workaround will just never be triggered again and all will be well.

(In reply to Alex Keybl [:akeybl] from comment #13)
> We've also heard the Gallery app doesn't appear to be affected by this for
> pictures taken from the Camera anymore - would be good to confirm. They may
> be relying on filename right now.

That would be good to confirm. Because there is a workaround in place, this bug is kind of hard to observe.  I have no idea what you mean by "relying on filename" however.  This bug doesn't have anything to do with filenames.

Comment 15

[Doug Turner (:dougt)]

Attachment: patch v.3

Ben, you'll see two comments in indexedDB.  Look for /*aaaaa*/.  I am not sure if we have the file time in either of these places.  Do you?

Comment 16

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

Comment on attachment 672359 [details] [diff] [review]
patch v.3

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

r=me with these things fixed.

Note that the IDBObjectStore.cpp is wrong but it's fixed in khuey's patch, so the two need to either land together or we need to merge.

::: content/base/public/nsDOMFile.h
@@ +71,5 @@
>    }
>  
> +  bool IsDateUnknown() const
> +  {
> +    return mLastModificationDate == UINT64_MAX;

I think this is actually wrong... This will always return true for blobs (i.e. not files), and I think that it shouldn't. You should probably do:

  return mIsFile && mLastModificationDate == UINT64_MAX;

Right?

::: dom/indexedDB/IDBObjectStore.cpp
@@ +618,5 @@
>  
>  inline
>  bool
>  ResolveMysteryBlob(nsIDOMBlob* aBlob, const nsString& aContentType,
> +                   uint64_t aSize, uint64_t aLastModifiedDate)

This change isn't right, blobs (i.e. not files) don't have a last-mod date. However, we caught this in khuey's patch so let's leave this alone so we don't have to merge.

::: dom/ipc/Blob.cpp
@@ +590,5 @@
> +  {
> +    mImmutable = true;
> +  }
> +
> +  RemoteBlob(const nsAString& aName, const nsAString& aContentType,

Hm, instead of adding another constructor can you just change the existing one? I don't want there to be multiple constructors here beyond one for blobs and one for files.

@@ +660,5 @@
>      return static_cast<typename ActorType::ProtocolType*>(mActor);
>    }
> +
> +  NS_IMETHOD
> +  GetLastModifiedDate(JSContext* cx, JS::Value *aLastModifiedDate)

Nit: * on the left.

@@ +799,5 @@
>    MOZ_ASSERT(NS_IsMainThread());
>    MOZ_ASSERT(mBlob);
>    MOZ_ASSERT(mRemoteBlob);
>    MOZ_ASSERT(aLength);
> +  MOZ_ASSERT(aLastModifiedDate);

Instead of asserting non-zero you should assert that it isn't UINT64_MAX.

::: dom/ipc/ContentChild.cpp
@@ +533,5 @@
>    const nsDOMFileBase* blob = static_cast<nsDOMFileBase*>(aBlob);
>  
>    BlobConstructorParams params;
>  
> +  if (blob->IsSizeUnknown() || blob->IsDateUnknown()) {

If you changed IsDateUnknown like I suggested then this is ok. If not then you need to do an extra check to make sure that this isn't a non-file blob because that never knows the date.

::: dom/ipc/ContentParent.cpp
@@ +1237,5 @@
>    const nsDOMFileBase* blob = static_cast<nsDOMFileBase*>(aBlob);
>  
>    BlobConstructorParams params;
>  
> +  if (blob->IsSizeUnknown() || blob->IsDateUnknown()) {

Same comment here.

Comment 17

[Doug Turner (:dougt)]

https://hg.mozilla.org/mozilla-central/rev/cf1bbed46731
https://hg.mozilla.org/releases/mozilla-aurora/rev/00be9983e8c2

Comment 18

[Doug Turner (:dougt)]

needed to back this cset out of m-a due to a build bustage:
   https://hg.mozilla.org/releases/mozilla-aurora/rev/74671b2b40ec

Comment 19

[Doug Turner (:dougt)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f9b8841ec02a