782352 - Device Storage - track local io and broadcast onchange notifications

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 2

[Doug Turner (:dougt)]

One problem I found was if you have different device storage objects.  Suppose pictures and video.  Then a change happens in video.  pictures will send up seeing it.  We need to guard against that.  I'll do that work here.  Should be done tonight/tomorrow.

Comment 3

[Doug Turner (:dougt)]

ignore comment #2.  This isn't a problem now.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

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

I think I'm misunderstanding how the permissions around notifications are working. Would like to understand that better. r=me in the meantime.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +66,5 @@
> +    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +    nsString data;
> +    CopyASCIItoUTF16(mType, data);
> +    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +    obs->NotifyObservers(mFile, "file-watcher-update", data.get());    

Nit: trailing whitespace

@@ +185,5 @@
>      return rv;
>    }
>  
> +  nsCOMPtr<IOEventComplete> iocomplete = new IOEventComplete(mFile, "created");
> +  NS_DispatchToMainThread(iocomplete);    

Nit: more trailing whitespace

@@ +265,5 @@
> +DeviceStorageFile::Remove()
> +{
> +  mFile->Remove(true);
> +  nsCOMPtr<IOEventComplete> iocomplete = new IOEventComplete(mFile, "deleted");
> +  NS_DispatchToMainThread(iocomplete);    

Same here

@@ +1332,5 @@
>        }
>  
>        case DEVICE_STORAGE_REQUEST_WATCH:
>        {
> +	// do something?

Don't you need to set some flag here to indicate that sending out notifications is ok? The mIsWatchingFile flag?

@@ +1824,3 @@
>        return NS_OK;
>      }
> +    Notify(creason.get(), file);

You can just do Notify(NS_ConvertUTF16toUTF8(aData)).get(), file)

There's no other advantages than simpler syntax.

Comment 5

[Doug Turner (:dougt)]

the permission stuff for onchange happend in 782391.  It depends on this bug.

Comment 6

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77d2640d9892

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/77d2640d9892

Comment 8

[David Flanagan [:djf]]

So it turns out that the words "created" and "modified" mean something different depending on how far down or up the stack you go...

You're using "created" to mean that an empty file has been created and "modified" to mean that bytes have been written into that file.  Those two events always arrive in closely spaced pairs.

To me, "created" means "I have finished writing bytes to a file, and no file by that name existed before I started".  And "modified" means "I have finished writing into a file that already existed".

Given what you've implemented here, I'll ignore the created notifications and just use the "modified" notifications.  But I'll have to check my own database to see if I already know about the file.  If so, I'll treat it as a deletion followed by a creation.  Otherwise, I'll just treat it as a creation.

I'm not going to reopen this bug myself, but I suspect you might want to. It seems to me that exposing the path of a file that is actively being written (by sending the created event before the modified event) just opens the door to race conditions and other ugliness.

I'd love to have an API that makes my kind of distinction between created and modified.  But I don't need to have it.  I'd suggest, however, that the distinction you've made is not useful and could actually be dangerous. So at a minimum, I'd suggest you drop the 'created' notifications.

Comment 9

[David Flanagan [:djf]]

This patch broadcasts "deleted" change events whenever delete() is called, even if no file by that name existed.  That seems like a bug. (or a general-purpose interapp communication mechanism...)

Comment 10

[Doug Turner (:dougt)]

reopening so that I remember to address these issues.

Comment 11

[Doug Turner (:dougt)]

Attachment: patch v.1 - followup

followup that prevents Remove() from ever posting an event during failure.

Comment 12

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

Comment on attachment 654447 [details] [diff] [review]
patch v.1 - followup

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

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +263,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
>  DeviceStorageFile::Remove()

Can you add an assertion to this function indicating whether it should run on or off the main thread?

@@ +267,5 @@
>  DeviceStorageFile::Remove()
>  {
> +  bool check;
> +  nsresult rv = mFile->Exists(&check);
> +  

Extra whitespace.

@@ +268,5 @@
>  {
> +  bool check;
> +  nsresult rv = mFile->Exists(&check);
> +  
> +  if (NS_FAILED(rv)) {

if (NS_FAILED(rv) || ! check) {
  return rv;
}

Should be ok, since if the !check fails then rv was NS_OK.

@@ +271,5 @@
> +  
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +  

Extra whitespace.

@@ +286,2 @@
>    nsCOMPtr<IOEventComplete> iocomplete = new IOEventComplete(this, "deleted");
>    NS_DispatchToMainThread(iocomplete);    

Extra whitespace at EOL.

Comment 13

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87a187376b3c

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/87a187376b3c