Last Comment Bug 782352 - Device Storage - track local io and broadcast onchange notifications
: Device Storage - track local io and broadcast onchange notifications
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Doug Turner (:dougt)
:
Mentors:
Depends on:
Blocks: 777101
  Show dependency treegraph
 
Reported: 2012-08-13 11:07 PDT by Doug Turner (:dougt)
Modified: 2012-08-29 06:39 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v.1 (25.84 KB, patch)
2012-08-13 15:52 PDT, Doug Turner (:dougt)
jonas: review+
Details | Diff | Review
patch v.1 - followup (1.49 KB, patch)
2012-08-22 17:24 PDT, Doug Turner (:dougt)
khuey: review+
Details | Diff | Review

Description Doug Turner (:dougt) 2012-08-13 11:07:08 PDT

    
Comment 1 Doug Turner (:dougt) 2012-08-13 15:52:41 PDT
Created attachment 651560 [details] [diff] [review]
patch v.1
Comment 2 Doug Turner (:dougt) 2012-08-16 16:36:52 PDT
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) 2012-08-16 21:56:36 PDT
ignore comment #2.  This isn't a problem now.
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2012-08-17 15:22:50 PDT
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) 2012-08-17 16:49:09 PDT
the permission stuff for onchange happend in 782391.  It depends on this bug.
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-08-18 04:25:43 PDT
https://hg.mozilla.org/mozilla-central/rev/77d2640d9892
Comment 8 David Flanagan [:djf] 2012-08-22 15:44:19 PDT
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] 2012-08-22 15:53:49 PDT
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) 2012-08-22 16:38:48 PDT
reopening so that I remember to address these issues.
Comment 11 Doug Turner (:dougt) 2012-08-22 17:24:05 PDT
Created attachment 654447 [details] [diff] [review]
patch v.1 - followup

followup that prevents Remove() from ever posting an event during failure.
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-08-28 10:03:20 PDT
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 14 Ed Morley [:emorley] 2012-08-29 06:39:19 PDT
https://hg.mozilla.org/mozilla-central/rev/87a187376b3c

Note You need to log in before you can comment on or make changes to this bug.