Device Storage - track local io and broadcast onchange notifications

RESOLVED FIXED in mozilla17

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

unspecified
mozilla17
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Comment hidden (empty)
(Assignee)

Updated

5 years ago
Blocks: 777101
(Assignee)

Updated

5 years ago
Summary: device Storage - track local io and broadcast onchange notifications → Device Storage - track local io and broadcast onchange notifications
(Assignee)

Comment 1

5 years ago
Created attachment 651560 [details] [diff] [review]
patch v.1
Attachment #651560 - Flags: review?(jonas)
(Assignee)

Comment 2

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

Comment 3

5 years ago
ignore comment #2.  This isn't a problem now.
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.
Attachment #651560 - Flags: review?(jonas) → review+
(Assignee)

Comment 5

5 years ago
the permission stuff for onchange happend in 782391.  It depends on this bug.
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/77d2640d9892
https://hg.mozilla.org/mozilla-central/rev/77d2640d9892
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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.
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...)
(Assignee)

Comment 10

5 years ago
reopening so that I remember to address these issues.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

5 years ago
Created attachment 654447 [details] [diff] [review]
patch v.1 - followup

followup that prevents Remove() from ever posting an event during failure.
Attachment #654447 - Flags: review?(jonas)
(Assignee)

Updated

5 years ago
Attachment #654447 - Flags: review?(jonas) → review?(khuey)
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.
Attachment #654447 - Flags: review?(khuey) → review+
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/87a187376b3c
https://hg.mozilla.org/mozilla-central/rev/87a187376b3c
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.