345852 - Write out additions to personal dictionary to file immediately and not at the and of the session

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Ria Klaassen (not reading all bugmail)]

1. In a text field, type: Firefox
2. Right-click on the word, choose "Add to dictionary"
3. Crash Firefox
4. Start Firefox
5. Go to a text field and type: Firefox
6. The word will show up with a red underline

Comment 1

[Adam Guthrie]

I thought we lacked the ability to do this in the first place... see bug 338291.

Comment 2

[Brett Wilson]

We have a custom dictionary that we implemented on a higher level than myspell. Apparently, it only gets written when the app shuts down, so additions are lost if we crash.

Comment 3

[Brett Wilson]

Not going to get fixed for Fx2.

Comment 4

[Wayne Mery (:wsmwk)]

still loses additions as described after crash
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081106 Minefield/3.1b2pre

But, Thunderbird doesn't lose the dictionary additions
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081104 Shredder/3.0b1pre

Comment 6

[Jorg K (CEST = GMT+2)]

The fix would be to write out the dictionary additions to the file immediately (as suggested somewhere in the description of bug 618805 comment #0 (quote: "When using "Add to dictionary" the selected word should be added immediately in persdict.dat")

Comment 7

[Harald-René Flasch]

Vote +1 for "(quote: "When using "Add to dictionary" the selected word should be added immediately in persdict.dat")"

This should be implemented as an option/setting so to give the users the choice to select the desired behavior.

--hfrmobile

Comment 8

[Harald-René Flasch]

Btw, the original report is from 2006 ... Maybe this is another reason why I switched to Chrome ...

Comment 9

[Jorg K (CEST = GMT+2)]

Actually, Firefox hasn't crashed on me for a long time.

Personally, I don't find the "personal" dictionary so useful. It would be more useful, if there were one per language (bug 715168). I think these to bugs should be addressed together.

Comment 10

[Harald-René Flasch]

Agreed, but this would more a feature request, than a bug.

But I doubt that Mozilla will change this since the used engine (ASpell) is used also by other Apps (Notepad++, Chrome, Microsoft Word, ...)

I guess that the personal dictionary was introduced for "personal name", "acronyms" etc. which are the same in all languages (e.g. ASpell, Firefox, Chrome, Microsoft, Mozilla etc.).

Maybe you find my "UserDict Synchronizer" useful which synchronizes the personal dictionary between several apps so that you have to enter a "personal name" only once :)

http://hfrmobile.com/apps/app_UserDictSync_T1/index.htm

--hfrmobile

Comment 11

[Jorg K (CEST = GMT+2)]

As far as I can see, it's all Mozilla code:
Dictionary is saved in the destructor of the mzSpellChecker:
http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozSpellChecker.cpp#46

Endsession:
http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozPersonalDictionary.cpp#356

Save:
http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozPersonalDictionary.cpp#250

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: simple fix to write out the dictionary when a word is added/removed

This there more to it than this simple patch?
Do we need a test for this? How would I go about it?
Like always, the test would be harder than the fix ;-((

Comment 13

[(no longer active)]

Comment on attachment 8607213 [details] [diff] [review]
simple fix to write out the dictionary when a word is added/removed

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

This is unfortunately not the write fix for this.  The reason is that writing this dictionary happens on the main thread, and doing I/O on the main thread is a big no-no (since it will freeze the UI for potentially hundreds of milliseconds, if not seconds on slower systems.)  As things stand right now, we do this work once when we're shutting down, which is not great, but still not terrible, but this patch will cause UI jank every time you add a new word to the user dictionary (for example through the context menu for text boxes), which is not acceptable.

The right fix for this is a bit more involved.  You need to move the write to disk to a background thread.  If you look at bug 880864, where we started to load this file off the main thread, you can get a pretty good idea on what we need to do here.

Here is a rough sketch of the fix:

* In AddWord and RemoveWord, call Save().
* Modify Save() by constrcting the array from mDictionaryTable, and then construct a runnable object similar to the one added in bug 880864, pass the array to it, and inside its Run() function, do the actual I/O to write down the array.
* Add a Monitor similar to the one added in bug 880864, set it from the runnable when the write is done, add a WaitForSave() function, and call it in the Save() function before creating the word array.  This will ensure that if a new save request arrives before the previous one is finished, we wait on it, and not attempt to write to the file before the last write was finished.
* Get rid of the mDirty flag, since it won't be useful any more.

Let me know if you have questions!

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: Solution option 1: No passing of word list

Option 2 follows in a minute ;-)

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: Solution option 2: pass word list to the other thread

OK, here I have two solutions.

The first solution is simpler. The waiting for save is done before adding/removing a word from the list, thus we don't disturb the list while it is still being written out.

The second solution is as Ehsan wanted it. A copy (I hope I got this right, since I know close to nothing about the nsTArray<nsString> stuff) is passed to the class doing the writing, and the waiting happens in the Save function *after* words were added/removed from the list.

I'm not sure that the second solution is needed. We will always only ever have one write going at any given time, so if the users adds words to the dictionary rapidly, there will be a wait until the first write has finished.

Given that, the first and the second solution will have the same user delay. So I prefer the simpler first solution.

Sorry if I got the monitor stuff wrong, it's using the same monitor that the load is using.

Since Ehsan invited questions: Tell me what's wrong with the two solutions ;-)

Comment 16

[(no longer active)]

Comment on attachment 8608345 [details] [diff] [review]
Solution option 1: No passing of word list

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

It seems like this is just copying what we do for the load case without paying any attention to what I wrote in comment 13.

Comment 17

[(no longer active)]

Comment on attachment 8608352 [details] [diff] [review]
Solution option 2: pass word list to the other thread

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

::: extensions/spellcheck/src/mozPersonalDictionary.cpp
@@ +94,5 @@
> +    mDict.forget(&dict);
> +
> +    nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
> +    if (mainThread) {
> +      NS_ProxyRelease(mainThread, static_cast<mozIPersonalDictionary *>(dict));

You don't really need to access the mozPersonalDictionary object at all from the runnable object, so this stuff is unnecessary.  Again, it seems like you're copying what we're doing in the load code path, which is not what I suggested in comment 13.  More specifically, the part about copying the array was intended so that you don't have to deal with accessing the object from multiple threads.

If you have questions about comment 13, please ask them.  This is code dealing with multiple threads, so it's very important for you to understand exactly what needs to happen, and copying what other code does like this is kind of dangerous.  Please ping me on IRC if you need to discuss things, that will lead to results much faster than implementing random ideas in patches and asking for feedback on them.

Thanks!

Comment 18

[Jorg K (CEST = GMT+2)]

That's a bit harsh. Yes, I did some copying of the load case. I *did* pay attention to your comment. Let's see:

> * In AddWord and RemoveWord, call Save().
Yes. Done.

> * Modify Save() by constrcting the array from mDictionaryTable, and then construct a
> runnable object similar to the one added in bug 880864, pass the array to it, and inside 
> its Run() function, do the actual I/O to write down the array.
Done in option 2, although you can still enlighten me why this is actually necessary.

So, as you said: 
> * Add a Monitor similar to the one added in bug 880864, set it from the runnable
> when the write is done, add a WaitForSave() function, and call it in the Save()
> function before creating the word array.
Hmm, reused the monitor that was there.

> * Get rid of the mDirty flag, since it won't be useful any more.
Done that.

> without paying *any* attention to what I wrote in comment 13.
Well, I don't think so, at least I paid 75% attention ;-)

Comment 19

[Jorg K (CEST = GMT+2)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #17)
> You don't really need to access the mozPersonalDictionary object at all from
> the runnable object, ... . 

Well, if that's the strategy, I'll look at it again.

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: WIP

I don't know how we would discuss things on IRC, if we don't see the code we're talking about.

I did what you said, I moved all the writing to disk into the runnable.

The code compiles but is incomplete. Questions:
1) Is there some documentation on what this monitor stuff actually does?
   It's a little tedious to try to reverse engineer it or ask on IRC.
2) As you can see, I commented out the WaitforSave function.
   The original was communicating via a member of the
   mozPersonalDictionary object. Now that the runnable doesn't access the
   dictionary object any more, I don't know who write the WaitforSave function.

Perhaps you can give me a code snippet for the WaitforSave function, that would lead to results much faster ;-)

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: WIP 2

This is as good as I can do without knowing how the monitor stuff works. I've programmed IPC (semaphores and such) long ago and in a very different context. You seem to be an expert on the monitor stuff:
https://github.com/ehsan/mozilla-history/blob/master/ipc/glue/AsyncChannel.cpp

Maybe/most likely the 'static' is not right, if it runs in different threads. You said:
> Add a Monitor similar to the one added in bug 880864, set it from the runnable ...
Set it? How?

I don't understand your:
> add a WaitForSave() function, and call it in the Save() function before
> creating the word array

The Save runs on the main thread, and it ships off the saving to its little helpers in other threads. Why would the main thread need to wait? Only the helpers amongst each other need to use a semaphore to coordinate the access to the file.

Comment 22

[(no longer active)]

(In reply to Jorg K from comment #18)
> That's a bit harsh. Yes, I did some copying of the load case. I *did* pay
> attention to your comment. Let's see:

Sorry, I didn't mean for it to be harsh.  :-)

> > * In AddWord and RemoveWord, call Save().
> Yes. Done.
> 
> > * Modify Save() by constrcting the array from mDictionaryTable, and then construct a
> > runnable object similar to the one added in bug 880864, pass the array to it, and inside 
> > its Run() function, do the actual I/O to write down the array.
> Done in option 2, although you can still enlighten me why this is actually
> necessary.

It's usually safer to just give a background thread a copy of data that it needs to do some work and let it do the work independently of what the other thread is doing.  Otherwise you would need to worry about syncing up the data between the two threads.  My original idea was to make it unnecessary for the background thread to deal with the hashtable that is owned by the main thread object, but I just realized that we can't avoid accessing the mozPersonalDictionary object altogether since the monitor that we need to modify when we're done in the background thread belongs to that object...

The complication here is essentially caused by using the stream transport service threads for running the runnable.  This object implements a thread pool which means that if you dispatch two runnables to it, they may run on different threads concurrently, and that's why in Save(), we would need to wait for the previous save operation, if one is in progress to finish.  Otherwise, if we had a way to always use the same background thread, every time in Save(), we could create a new array based on the contents of our hashtable, send it off to the background thread as a new runnable, and we wouldn't need to wait on a monitor since the operations on the background thread would happen sequentially one by one.

Let me know if this is more clear now, or if you have more questions.

> So, as you said: 
> > * Add a Monitor similar to the one added in bug 880864, set it from the runnable
> > when the write is done, add a WaitForSave() function, and call it in the Save()
> > function before creating the word array.
> Hmm, reused the monitor that was there.

We can't reuse the monitors, unfortunately, since they signal different events.  If you use the same monitor, when you wait on it, you wouldn't be able to know why the wait was completed.

> > * Get rid of the mDirty flag, since it won't be useful any more.
> Done that.
> 
> > without paying *any* attention to what I wrote in comment 13.
> Well, I don't think so, at least I paid 75% attention ;-)

Hehe :-)  OK, fair!

(In reply to Jorg K from comment #20)
> Created attachment 8609075 [details] [diff] [review]
> WIP
> 
> I don't know how we would discuss things on IRC, if we don't see the code
> we're talking about.

People typically use pastebin/gist.  The advantage of IRC would be faster communication, so that you wouldn't have to wait for hours/days before I get back to you!

> I did what you said, I moved all the writing to disk into the runnable.
> 
> The code compiles but is incomplete. Questions:
> 1) Is there some documentation on what this monitor stuff actually does?
>    It's a little tedious to try to reverse engineer it or ask on IRC.

Hmm.  I don't think we have any specific documentation on our Monitor implementation, but monitors are a pretty well known synchronization primitive that are used in many different environments.  They basically provide a lock that can be used to synchronize access to a shared resource combined with a way to get notified when a lock becomes available, which means they can be implemented as a pair of mutex and condvar objects (that is exactly how mozilla::Monitor is implemented.)  Perhaps this will help? <http://en.wikipedia.org/wiki/Monitor_%28synchronization%29>

(If you're familiar with mutexes and condvars, just reading the implementation here should be enough, it's a pretty thin layer <https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/Monitor.h#24>.)

> 2) As you can see, I commented out the WaitforSave function.
>    The original was communicating via a member of the
>    mozPersonalDictionary object. Now that the runnable doesn't access the
>    dictionary object any more, I don't know who write the WaitforSave
> function.

Yeah, sorry, I was wrong on that part, as I mentioned above!

(In reply to Jorg K from comment #21)
> Created attachment 8609209 [details] [diff] [review]
> WIP 2
> 
> This is as good as I can do without knowing how the monitor stuff works.
> I've programmed IPC (semaphores and such) long ago and in a very different
> context. You seem to be an expert on the monitor stuff:
> https://github.com/ehsan/mozilla-history/blob/master/ipc/glue/AsyncChannel.
> cpp
> 
> Maybe/most likely the 'static' is not right, if it runs in different
> threads. You said:
> > Add a Monitor similar to the one added in bug 880864, set it from the runnable ...
> Set it? How?

Right now in your patch, WaitForSave is a member of mozPersonalDictionarySave.  It should be a member of mozPersonalDictionary.  With that, you hold a lock as you're currently doing in both Save() and the function called by the runnable to do the save in the background thread to ensure that they don't run concurrently, and you call mon.Notify() when the save operation is done.  You should use mon.Wait() to wait on the monitor.

> I don't understand your:
> > add a WaitForSave() function, and call it in the Save() function before
> > creating the word array
> 
> The Save runs on the main thread, and it ships off the saving to its little
> helpers in other threads. Why would the main thread need to wait? Only the
> helpers amongst each other need to use a semaphore to coordinate the access
> to the file.

The basic goal is to avoid writing to the file while there is a previous write in progress.  Also, every time that we want to write to the file, we need to have the latest word list.  Using the monitor as I suggested will solve both at once, whereas your idea above would only solve the former.

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: WIP 3: Should be pretty close to what Ehsan had in mind. Debug needs to be removed later.

Thank you for the suggestions. I took a look at monitors in general, together with wait, and notify. There are many implementations. Obviously the mozilla::MonitorAutoLock class that locks when a variable is declared and unlocks when it goes out of scope is a special Mozilla thing. This facility will block/sleep/wait the current thread until the lock can be obtained. Of course it helps to understand what I'm doing instead of copying other stuff without knowing what's going on ;-)

> you [should] hold a lock [...] in both Save() and the function called by the
> runnable to do the save in the background thread to ensure that they don't run
> concurrently, and you call mon.Notify() when the save operation is done. You
> should use mon.Wait() to wait on the monitor.

Given that mozilla::MonitorAutoLock already waits until the lock can be obtained and given that you said ...
> in Save(), we would need to wait for the previous save operation, 
> if one is in progress to finish.
... I don't see why the solution is any more complicated as what is proposed in this patch. No need for Wait() and Notify(). If the background save is still running, the Save() will wait for the monitor. The background tread will start, once the Save() is done and has released its lock.

Some debug shows this:
Case 1:
Word is added, no further action.
mozPersonalDictionary::Save: Will lock now
mozPersonalDictionary::Save: Locked!
mozPersonalDictionary::Save: Unlock and Exit
mozPersonalDictionarySave::Run(1bc92ac0): Will lock now
mozPersonalDictionarySave::Run(1bc92ac0): Locked!
mozPersonalDictionarySave::Run(1bc92ac0): Processing ...
mozPersonalDictionarySave::Run(1bc92ac0): Processing done.
mozPersonalDictionarySave::Run(1bc92ac0): Unlock.
mozPersonalDictionarySave::Run(1bc92ac0): Exit.

Case 2:
Adding a second word while the other thread is still processing.
mozPersonalDictionary::Save: Will lock now
mozPersonalDictionary::Save: Locked!
mozPersonalDictionary::Save: Unlock and Exit
mozPersonalDictionarySave::Run(1549f2a0): Will lock now
mozPersonalDictionarySave::Run(1549f2a0): Locked!
mozPersonalDictionarySave::Run(1549f2a0): Processing ...
  -- User adds another word.
mozPersonalDictionary::Save: Will lock now
  -- Main thread is waiting for the lock and hence unresponsive.
mozPersonalDictionarySave::Run(1549f2a0): Processing done.
mozPersonalDictionarySave::Run(1549f2a0): Unlock.
mozPersonalDictionary::Save: Locked!
mozPersonalDictionarySave::Run(1549f2a0): Exit.
mozPersonalDictionary::Save: Unlock and Exit
mozPersonalDictionarySave::Run(16c418a0): Will lock now
mozPersonalDictionarySave::Run(16c418a0): Locked!
mozPersonalDictionarySave::Run(16c418a0): Processing ...
mozPersonalDictionarySave::Run(16c418a0): Processing done.
mozPersonalDictionarySave::Run(16c418a0): Unlock.
mozPersonalDictionarySave::Run(16c418a0): Exit.

The unresponsiveness of the main thread can only be avoided by not waiting in the main thread, but you said we need to wait in order to "serialise" the writes since the background threads are run in a pool in no particular order. I think however, it would be very difficult to cause such a condition. The user would have to add two words or undo the addition so quickly that the second background thread would - by chance - be processed before the first.

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: WIP 4: Here's a vastly better solution.

Much better solution. The save request receives a sequence number. This way we don't have to wait in the Save() function at all. The user can add/remove words in rapid succession. When the system catches up, it will discard all requests which don't match the last request issued. This way, the thread pool can process out of order, only the most current one will be processed.

The debug shows how it works:
  -- User adds a word
mozPersonalDictionary::Save: Dispatching save with sequence 1
mozPersonalDictionarySave::Run(1179dfe0): Will lock now for request 1
mozPersonalDictionarySave::Run(1179dfe0): Locked for request 1!
mozPersonalDictionarySave::Run(1179dfe0): Processing request 1 (1) ...
mozPersonalDictionarySave::Run(1179dfe0): Processing done.
mozPersonalDictionarySave::Run(1179dfe0): Unlock.
mozPersonalDictionarySave::Run(1179dfe0): Exit.

  -- User adds six more words in rapid succession.
  -- Main thread doesn't block at all, as all save requests are immediately
  -- dispatched.
mozPersonalDictionary::Save: Dispatching save with sequence 2
mozPersonalDictionarySave::Run(1163ac60): Will lock now for request 2
mozPersonalDictionarySave::Run(1163ac60): Locked for request 2!
mozPersonalDictionarySave::Run(1163ac60): Processing request 2 (2) ...
mozPersonalDictionary::Save: Dispatching save with sequence 3
mozPersonalDictionarySave::Run(11648100): Will lock now for request 3
mozPersonalDictionary::Save: Dispatching save with sequence 4
mozPersonalDictionarySave::Run(1165d380): Will lock now for request 4
mozPersonalDictionary::Save: Dispatching save with sequence 5
mozPersonalDictionarySave::Run(11bff180): Will lock now for request 5
mozPersonalDictionary::Save: Dispatching save with sequence 6
mozPersonalDictionarySave::Run(11fdc900): Will lock now for request 6
mozPersonalDictionary::Save: Dispatching save with sequence 7
mozPersonalDictionarySave::Run(125b8bc0): Will lock now for request 7
mozPersonalDictionarySave::Run(1163ac60): Processing done.
mozPersonalDictionarySave::Run(1163ac60): Unlock.
mozPersonalDictionarySave::Run(1163ac60): Exit.
  -- Processing of second addition is done, monitor becomes available for subsequent additions.
  -- All requests are discarded ...
mozPersonalDictionarySave::Run(11648100): Locked for request 3!
mozPersonalDictionarySave::Run(11648100): Processing request 3 (7) ...
mozPersonalDictionarySave::Run(11648100): Discarding non-current request.
mozPersonalDictionarySave::Run(11648100): Exit.
mozPersonalDictionarySave::Run(1165d380): Locked for request 4!
mozPersonalDictionarySave::Run(1165d380): Processing request 4 (7) ...
mozPersonalDictionarySave::Run(1165d380): Discarding non-current request.
mozPersonalDictionarySave::Run(1165d380): Exit.
mozPersonalDictionarySave::Run(11bff180): Locked for request 5!
mozPersonalDictionarySave::Run(11bff180): Processing request 5 (7) ...
mozPersonalDictionarySave::Run(11bff180): Discarding non-current request.
mozPersonalDictionarySave::Run(11bff180): Exit.
mozPersonalDictionarySave::Run(11fdc900): Locked for request 6!
mozPersonalDictionarySave::Run(11fdc900): Processing request 6 (7) ...
mozPersonalDictionarySave::Run(11fdc900): Discarding non-current request.
mozPersonalDictionarySave::Run(11fdc900): Exit.
  -- until finally we get to the last request issued. This one is processed.
mozPersonalDictionarySave::Run(125b8bc0): Locked for request 7!
mozPersonalDictionarySave::Run(125b8bc0): Processing request 7 (7) ...
mozPersonalDictionarySave::Run(125b8bc0): Processing done.
mozPersonalDictionarySave::Run(125b8bc0): Unlock.
mozPersonalDictionarySave::Run(125b8bc0): Exit.

Finally at the end of the session, we see this:
mozPersonalDictionary::Save: Dispatching save with sequence 8
mozPersonalDictionarySave::Run(5458d40): Will lock now for request 8
mozPersonalDictionarySave::Run(5458d40): Locked for request 8!
mozPersonalDictionarySave::Run(5458d40): Processing request 8 (8) ...
  -- I didn't catch the rest since the window closed.

Not sure why it's saved again when the application closes. Must come from mozPersonalDictionary::Observe. Maybe the Save() should be removed there. I've already removed it from mozPersonalDictionary::EndSession.

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: WIP 4a: Here's a vastly better solution (revised)

Removed an incorrect comment.
Please ignore the print "mozPersonalDictionarySave::Run(...): Unlock."
It was printed at the wrong spot.

Comment 26

[(no longer active)]

Comment on attachment 8609758 [details] [diff] [review]
WIP 4a: Here's a vastly better solution (revised)

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

I'm so sorry for the terrible turn around time!  Will do better in the future. :-)

::: extensions/spellcheck/src/mozPersonalDictionary.cpp
@@ +81,5 @@
> +{
> +public:
> +  explicit mozPersonalDictionarySave(mozPersonalDictionary *aDict, nsCOMPtr<nsIFile> aFile, nsTArray<nsString> &&aDictWords)
> +    : mDictWords(aDictWords),
> +      mFile (aFile),

Nit: no space before parenthesis, please.

@@ -251,5 @@
>  {
>    nsCOMPtr<nsIFile> theFile;
>    nsresult res;
> -
> -  WaitForLoad();

Why did you remove this?  That shouldn't be needed.

@@ -252,5 @@
>    nsCOMPtr<nsIFile> theFile;
>    nsresult res;
> -
> -  WaitForLoad();
> -  if(!mDirty) return NS_OK;

If I'm not missing anything, removing mDirty now means that you could end up with two saves of the exact same data because we don't remember if we have changed something from the last time.  Perhaps you should maintain this flag, or address that problem some other way.

@@ +350,5 @@
>  
> +  mSequence++;
> +  if (mSequence == 100000) {
> +    // Don't overflow, let's hope we don't have a backlog of that many save requests.
> +    mSequence = 0;

I think this is way too complicated, and it's also wrong as it's written.  This code needs to work no matter how many times it's called.

Here is what I would like to see: Before you start a save operation, wait on the monitor to make sure no concurrent save is in progress.  Then hand off the stuff to write to a background thread.  When the work of writing them to disk is done, notify the monitor.

Because we also have async loading, we would need to make sure no concurrent loads and saves are in progress.  The easiest way to do that would be to use the same monitor.

@@ -356,5 @@
>  NS_IMETHODIMP mozPersonalDictionary::EndSession()
>  {
>    WaitForLoad();
>  
> -  Save(); // save any custom words at the end of a spell check session

Removing this will lose the final modifications, wouldn't it?  Assuming I'm not missing anything, please add it back.

::: extensions/spellcheck/src/mozPersonalDictionary.h
@@ +16,5 @@
>  #include "nsCRT.h"
>  #include "nsCycleCollectionParticipant.h"
>  #include "nsHashKeys.h"
>  #include <mozilla/Monitor.h>
> +#include <mozilla/Move.h>

You don't need this include in the header file.  Please move it to the cpp.

Comment 27

[Jorg K (CEST = GMT+2)]

It's been a long time, so we both have to get up to speed with this again ;-)

WaitForLoad() was removed in Save() since Save() is only called from AddWord and RemoveWord where WaitForLoad() was already called *before* Save() is called. So by the time Save() is called, the dictionary is well and truly loaded initially.

Let me now describe the solution, so you can see that I can't follow any of your other remarks (apart from the nits (space before parenthesis and inclusion)):

When the user adds or removes a word from the dictionary, the changed list is stored immediately. Therefore no mDirty flag is required. You said yourself in comment #13: "Get rid of the mDirty flag, since it won't be useful any more". Also, no saving is necessary in EndSession.

Since the loading happens once and never again, and the program actually waits for the loading to complete, I don't think it's a good idea to use the same monitor for loading and saving. This could be done (perhaps), but it complicates the solution.

Here is how the Save() is working. It basically grabs the word list and ships it off to an asynchronous runnable. Before it does, it remembers the number of the latest request in a member variable of the dictionary. No waiting happens. When the next save request arrives, another runnable is created.

In this way, a pool of runnables is created, which all compete for the same resource: access to the file they want to write. So the runnables all try to lock the same monitor and wait until the lock is obtained. When the runnable finally obtains the lock, it checks first, whether the write request is still current, that is, whether the sequence number is the write request is still the highest sequence number that the dictionary holds. If not, the write request is discarded. Otherwise, the write request is executed. You can see this nicely in my comment #24. While write request 2 is being executed (and artificially delayed by 15sec sleep), the user has added/remove words, to more requests were created, in the example, up to number 7. When request 3, 4, 5 and 6 come to execute, they get discarded, only 7 gets executed. Note that algorithm would also work "out of order", so if 7 came to execute first, the others would be discarded. in summary: Only the write request is executed whose number matches the write request stored in the dictionary object, all others are discarded.

One word about incrementing the request number. Obviously we don't don't want to overflow the request number should the user do a lot of modifications (more than 2^31) to the dictionary in one session.

So I roll the request number around from 100,000 back to 0. I have to admit, that this is the bit, that's not so elegant in the solution. Let's assume we wrap around at 2 instead of 100,000. So request 0 is created and executed. It gets executed so slowly that in the meantime new requests 1(a), 2 and then 0 and 1(b) get created. Upon execution, requests 2 and 0 would be discarded, and the two requests 1(a) and 1(b) would be executed, which is not a problem, unless the are executed "out of order", so 1(a) after 1(b). So rolling around to 0 from N would only a problem, if the delay in execution were so large so that the pool of waiting write requests were larger than N and execution happened "out of order". I think we can safely assume that this won't be the case. But we could of course wrap at MAXINT (32 bit) instead.

Having said all that, I'll come back to what you said:
> Here is what I would like to see: Before you start a save operation, wait on the monitor to make
> sure no concurrent save is in progress.  Then hand off the stuff to write to a background thread.
> When the work of writing them to disk is done, notify the monitor.

I've already tried this in WIP 3, comment #23: This can cause (unlikely) freezes in the UI. If the user changes the dictionary while the previous save is not complete, the UI is frozen, until the save completes. WIP 3 is basically the same as WIP 4, but without the sequence number and therefore with additional locking in the Save() function itself. You can directly compare WIP 3 and WIP 4. If WIP 3 is the solution you want, let's go with it.

Comment 28

[Jorg K (CEST = GMT+2)]

Comment on attachment 8609745 [details] [diff] [review]
WIP 3: Should be pretty close to what Ehsan had in mind. Debug needs to be removed later.

Made WIP 3 visible again. Less elegant but less complicated than WIP 4.

Comment 29

[Jorg K (CEST = GMT+2)]

In the meantime I've thought about the two solutions some more. WIP 4 is really far more elegant, since the locking and waiting only happens in the runnable, the Save() function never waits.

The only blemish this solution has is the sequence number that is incremented and then rolled over back to zero (which is unlikely). This can be fixed by using a time stamp instead of a sequence number, so the write request will only be executed if the write request carries the same time stamp as the last requests submitted to the pool by the Save() function.

Let me show you what I mean in another patch.

Comment 30

[Jorg K (CEST = GMT+2)]

Attachment: WIP 4b: Here's a vastly better solution using time stamps

This is WIP 4 but using a time stamp instead of a sequence number. The debug shows:

mozPersonalDictionary::Save: Dispatching save with timestamp 55a58350
mozPersonalDictionarySave::Run(a380960): Will lock now for request 55a58350
mozPersonalDictionarySave::Run(a380960): Locked for request 55a58350!
mozPersonalDictionarySave::Run(a380960): Processing request 55a58350, highest timestamp 55a58350
mozPersonalDictionarySave::Run(a380960): Processing done.
mozPersonalDictionarySave::Run(a380960): Exit.

While one request is exececuted (a8fb940/55a583af) four more are queued
(since the user busily adds words while the first addition hasn't been written out to disk):

mozPersonalDictionary::Save: Dispatching save with timestamp 55a583af
mozPersonalDictionarySave::Run(a8fb940): Will lock now for request 55a583af
mozPersonalDictionarySave::Run(a8fb940): Locked for request 55a583af!
mozPersonalDictionarySave::Run(a8fb940): Processing request 55a583af, highest timestamp 55a583af
mozPersonalDictionary::Save: Dispatching save with timestamp 55a583b1
mozPersonalDictionarySave::Run(ac3f220): Will lock now for request 55a583b1
mozPersonalDictionary::Save: Dispatching save with timestamp 55a583b4
mozPersonalDictionarySave::Run(ac7e440): Will lock now for request 55a583b4
mozPersonalDictionary::Save: Dispatching save with timestamp 55a583b7
mozPersonalDictionarySave::Run(adaec40): Will lock now for request 55a583b7
mozPersonalDictionary::Save: Dispatching save with timestamp 55a583bb
mozPersonalDictionarySave::Run(2e1e660): Will lock now for request 55a583bb
mozPersonalDictionarySave::Run(a8fb940): Processing done.
mozPersonalDictionarySave::Run(a8fb940): Exit.

Once the first request is finished, the other four are executed, but three of them are discarded:
mozPersonalDictionarySave::Run(ac3f220): Locked for request 55a583b1!
mozPersonalDictionarySave::Run(ac3f220): Processing request 55a583b1, highest timestamp 55a583bb
mozPersonalDictionarySave::Run(ac3f220): Discarding non-current request.
mozPersonalDictionarySave::Run(ac3f220): Exit.
mozPersonalDictionarySave::Run(ac7e440): Locked for request 55a583b4!
mozPersonalDictionarySave::Run(ac7e440): Processing request 55a583b4, highest timestamp 55a583bb
mozPersonalDictionarySave::Run(ac7e440): Discarding non-current request.
mozPersonalDictionarySave::Run(ac7e440): Exit.
mozPersonalDictionarySave::Run(adaec40): Locked for request 55a583b7!
mozPersonalDictionarySave::Run(adaec40): Processing request 55a583b7, highest timestamp 55a583bb
mozPersonalDictionarySave::Run(adaec40): Discarding non-current request.
mozPersonalDictionarySave::Run(adaec40): Exit.
mozPersonalDictionarySave::Run(2e1e660): Locked for request 55a583bb!
mozPersonalDictionarySave::Run(2e1e660): Processing request 55a583bb, highest timestamp 55a583bb
mozPersonalDictionarySave::Run(2e1e660): Processing done.
mozPersonalDictionarySave::Run(2e1e660): Exit.

You get the idea. The remaining problem is, that the time is measured in seconds, not milli-seconds. I'll fix that.

WIP 4 is my preferred solution.

Comment 31

[(no longer active)]

Comment on attachment 8633727 [details] [diff] [review]
WIP 4b: Here's a vastly better solution using time stamps

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

Timestamps that you get from ::time() are not monotonic and they can roll backwards if you change the system clock time, so your new patch will do the wrong thing in that case.

This functionality is accessible through our UI by right clicking on a misspelled word and clicking "Add to Dictionary".  The case where the main thread would have to wait for an ongoing Save() to finish is when the user manages to do the above twice in a row faster than we can write the file to the disk, and those events are several hundred milliseconds apart with a super fast person, and seconds or more apart with an ordinary user, so I think all of this complexity to avoid that potential main thread stall is not worth it in this case.  So I prefer to move forward with WIP 3.

Comment 32

[(no longer active)]

Comment on attachment 8609745 [details] [diff] [review]
WIP 3: Should be pretty close to what Ehsan had in mind. Debug needs to be removed later.

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

I think this is the right direction!  Many of my previous comments applies to this as well.  Do you mind addressing them please?  The patch should be ready for review after that.

(Also, I'd appreciate if you could remove the debugging changes as it makes it more difficult to understand what is going on.  Thanks!)

Comment 33

[Jorg K (CEST = GMT+2)]

Attachment: Chosen solution (WIP 3 with nits fixed).

Here is the chosen solution with the debug removed and two nits fixed:
Space before parenthesis and inclusion of Move.h.

I didn't address the following:

WaitForLoad() is removed from Save() since Save() is only called from AddWord and RemoveWord where WaitForLoad() was already called *before* Save() is called.

Two monitors are used so keep the Load and the Save apart. By the time we get to Save, the Load has well and truly happened, so there can't be any conflicts.

Save() was removed from EndSession, since it's not necessary there any more.

One last question remains: Why is Save() called in mozPersonalDictionary::Observe? I think that could be removed as well. In my debugging I observed that Save() was called when shutting down the session. This is unnecessary and must come from Observe.

Comment 34

[Jorg K (CEST = GMT+2)]

Confirmed:

xul.dll!mozPersonalDictionary::Save() Line 329	C++
xul.dll!mozPersonalDictionary::Observe(nsISupports * aSubject, const char * aTopic, const wchar_t * aData) Line 456	C++
xul.dll!nsObserverList::NotifyObservers(nsISupports * aSubject, const char * aTopic, const wchar_t * someData) Line 114	C++
xul.dll!nsObserverService::NotifyObservers(nsISupports * aSubject, const char * aTopic, const wchar_t * aSomeData) Line 319	C++
xul.dll!nsXREDirProvider::DoShutdown() Line 917	C++
xul.dll!ScopedXPCOMStartup::~ScopedXPCOMStartup() Line 1493	C++

Remove the Save() from Observe?

Comment 35

[Jorg K (CEST = GMT+2)]

I forgot to ask: Can you please suggest parameters for a try run. Thanks in advance.

Comment 36

[(no longer active)]

(In reply to Jorg K from comment #35)
> I forgot to ask: Can you please suggest parameters for a try run. Thanks in
> advance.

try: -b do -p all -t none -u all should give you all of the tests.

Comment 37

[(no longer active)]

Please don't remove Save() from anywhere.  If you want to do that, please file a follow-up and explain why you want to do it, and attach a separate patch.  Patches/bugs that do more than one thing are confusing to understand.

May I ask why you insist on keeping a separate monitor?

Comment 38

[Jorg K (CEST = GMT+2)]

Attachment: Chosen solution (WIP 3 with nits fixed, now using one monitor)

I used two monitors for clarity only. Here it the patch where Load and Save use the same monitor.

Save() was removed from EndSession, since it's not necessary there any more. This bug *is* about writing out the dictionary upon each change, not at the end of the session.

Save() was not removed from mozPersonalDictionary::Observe, since you insisted and since I don't know what this function is used for.
However, I think that it makes no sense to write out a dictionary that hasn't changed. Since I removed the mDirty flag that stopped short a Save() were no changes had occurred, the behaviour has changed now since Observe will always write out a new dictionary.
I don't agree with your assessment that it would be doing "more than one thing", since this patch is about reorganising the way the dictionary is written to disk in case changes have occurred. Removing unnecessary Save() calls are part of the bug. IMHO, it would be highly confusing to move this part to another bug.

I hope that settles all the issues. Please give removing Save() from Observe another thought.

Try run here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27a074359a6c

Comment 39

[Jorg K (CEST = GMT+2)]

Oops, that didn't compile on Linux and OS X, trying again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=614346a9fe12

Also, this try run is without Save() in Observe. It simply makes no sense to call the "new" Save() which unconditionally writes out a new dictionary, when the "old" Save() had a gate (mDirty) to avoid that.

Comment 40

[(no longer active)]

Hmm.  "profile-before-change" is a notification that we send out to all components to write the information that they need to persist to the disk before we shut down.  Therefore, since you have removed mDirty, it makes sense to remove that as well, otherwise we write to the disk every time during shutdown even if nothing has changed.  That being said, you should protect against an existing write to the disk going on when you receive profile-before-change, and the easiest way to do that seems to be calling WaitForSave() in Observe() instead of Save().

Comment 41

[Jorg K (CEST = GMT+2)]

Attachment: Chosen solution (WIP 3 with nits fixed, now using one monitor, Observe now also waits for completion of Save)

I'm glad you agree with me. If you look at the patch, you will see that there is no WaitForSave(). This is done by locking the monitor.

I changed the patch accordingly, now it's good to go.

I cancelled the try run and created another one with the latest code:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b91adbc35a3

You can review the patch once the try run comes out green.

Comment 42

[Jorg K (CEST = GMT+2)]

Comment on attachment 8634941 [details] [diff] [review]
Chosen solution (WIP 3 with nits fixed, now using one monitor, Observe now also waits for completion of Save)

Your mentioning of the WaitForSave function made it clear that WIP 3 isn't correct.

It's no good to lock the monitor (and wait there). There needs to be a WaitForSave function, like there was in WIP 2. Consequently there also needs to be a state variable to indicate whether a save is pending. We would wait forever if there was no save pending. The WaitForSave function can then be called in Save, EndSession and Observe.

I will submit a new patch later to cater for that. I've cancelled the try run.

Note that two monitors were used, since you said in comment #22:
> We can't reuse the monitors, unfortunately, since they signal different
> events. If you use the same monitor, when you wait on it, you wouldn't
> be able to know why the wait was completed.

The new patch will be based on WIP 2, with the state variable in the correct spot and of course *two* monitors. We're going around in circles here, but never mind ;-)

Comment 43

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution

This features:
- WaitForSave() used in Save(), EndSession() and Observe().
- Two monitors, so we know which one we've been waiting for.
- State variable "mSavePending", so we know whether to wait or not.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc7ead0e526c

Comment 44

[Jorg K (CEST = GMT+2)]

If you go back to comment #13 you'll see that the patch does *exactly* what you asked for. ;-)
Too bad it took me so long to understand all the details ;-(

Comment 45

[(no longer active)]

Oh boy!  Sorry for circling around like this.  And thanks for your perseverance!  :-)

Comment 46

[(no longer active)]

Comment on attachment 8635178 [details] [diff] [review]
Proposed solution

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

Looks great!  I have a few nits below, but I don't need to look at this again.  Please request checkin-needed when you're ready!

Thanks.

::: extensions/spellcheck/src/mozPersonalDictionary.cpp
@@ +24,5 @@
>  #include "nsIRunnable.h"
>  #include "nsThreadUtils.h"
>  #include "nsProxyRelease.h"
>  #include "prio.h"
> +#include <mozilla/Move.h>

Please use double quotes.

@@ +85,5 @@
>  
> +class mozPersonalDictionarySave final : public nsRunnable
> +{
> +public:
> +  explicit mozPersonalDictionarySave(mozPersonalDictionary *aDict, nsCOMPtr<nsIFile> aFile, nsTArray<nsString> &&aDictWords)

Nit: here and elsewhere in the patch, do you mind adjusting the indentation to ~80 columns?  Thanks!

@@ +92,5 @@
> +      mDict(aDict)
> +  {
> +  }
> +
> +  NS_IMETHOD Run()

Nit: please mark this as override.

@@ +113,5 @@
> +      }
> +
> +      uint32_t bytesWritten;
> +      nsAutoCString utf8Key;
> +      for (uint32_t i = 0; i < mDictWords.Length(); ++i ) {

Nit: no space before ).

@@ +340,5 @@
> +
> +  // The monitor has become available. This can have two reasons:
> +  // 1: The thread that does the save has finished.
> +  // 2: The thread that does the save hasn't even started. In this case we need
> +  //    to wait.  

Nit: trailing whitespace!

But more importantly these comments are great, and now WaitForLoad() is jealous!  Do you mind adding them to that function as well, please?

@@ +352,5 @@
>    nsCOMPtr<nsIFile> theFile;
>    nsresult res;
> +  
> +  WaitForSave();
> +  

Nit: more trailing whitespace here.

@@ +370,3 @@
>  
>    nsTArray<nsString> array(mDictionaryTable.Count());
>    mDictionaryTable.EnumerateEntries(AddHostToStringArray, &array);

Please rebase on top of bug 1182408 to make sure your patch applies cleanly when you request checkin-needed.

Comment 47

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (refreshed, nits fixed)

Carrying forward Ehsan's review+

Another (limited) try run to make sure that it's all good ;-)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=66570796c1ed

Comment 48

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48df0e843049

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/48df0e843049

Comment 50

[(no longer active)]

Thanks again, Jorg!