880864 - User dictionary (persdict.dat) read on main thread

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Vladan Djeric (:vladan)]

Attachment: Stack trace

We're reading the user's personal dictionary on the main thread shortly after startup. See attached stack

Comment 1

[(no longer active)]

Yes, ironically for initializing the location bar, which we don't spell check.  ;-)

I'm sure there's already a bug on file for this, probably filed by me...

Comment 2

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: User dictionary (persdict.dat) read on main thread, v1

Comment 3

[(no longer active)]

Comment on attachment 8400059 [details] [diff] [review]
User dictionary (persdict.dat) read on main thread, v1

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

I think spawning a thread for this is overkill.  Why can't we just do async IO on the file stream instead?

Comment 4

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: User dictionary (persdict.dat) read on main thread, v2

Comment 5

[(no longer active)]

Comment on attachment 8401193 [details] [diff] [review]
User dictionary (persdict.dat) read on main thread, v2

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

I'm sorry that I'm r-minusing your patch again...  But spinning the event loop is very dangerous, since it can lead into reentracy situations that we don't expect, and is a common source of sec-critical bugs.  I discussed this on IRC with Honza and he said that he has a better solution which he'll post about here.

Comment 6

[Honza Bambas (:mayhemer)]

I will mentor this bug.  :rvitillo, when will you work on this next?  I almost quit for today and will be back no sooner then on Monday.  Is it ok to give you the guidelines on Monday?

Comment 7

[Roberto Agostino Vitillo (:rvitillo)]

Monday sounds good, thanks.

Comment 8

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: User dictionary (persdict.dat) read on main thread, v23

Comment 9

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: User dictionary (persdict.dat) read on main thread, v3

Honza, is this what you had in mind?

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 8402678 [details] [diff] [review]
User dictionary (persdict.dat) read on main thread, v3

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

Looks pretty good!  Sorry for the delay, I'm quite busy these days.

some comments:

::: extensions/spellcheck/src/mozPersonalDictionary.cpp
@@ +46,5 @@
>  NS_INTERFACE_MAP_END
>  
>  NS_IMPL_CYCLE_COLLECTION_1(mozPersonalDictionary, mEncoder)
>  
> +class mozPersonalDictionaryLoader MOZ_FINAL : public nsIRunnable

make this a nested class of mozPersonalDictionary
derive from nsRunnable (no need to impl ISUPPORTS then)

@@ +58,5 @@
> +
> +  NS_IMETHOD Run()
> +  {
> +    MOZ_ASSERT(!NS_IsMainThread());
> +    return mDict->SyncLoad();

the result goes just nowhere...
I think SyncLoad() could be turned to void

@@ +62,5 @@
> +    return mDict->SyncLoad();
> +  }
> +
> +private:
> +  mozPersonalDictionary *mDict;

do a hard ref (nsRefPtr)

@@ +74,5 @@
>  }
>  
>  mozPersonalDictionary::~mozPersonalDictionary()
>  {
> +  EnsureDictIsLoaded();

if you do a hard ref, no need to this here.

@@ +89,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>    rv = svc->AddObserver(this, "profile-before-change", true);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  rv = Load();

The name should be StartLoad() or Preload()

When is Init() actually invoked?

@@ +90,5 @@
>    rv = svc->AddObserver(this, "profile-before-change", true);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  rv = Load();
> +  NS_ENSURE_SUCCESS(rv, rv);

Hmm.. before that the result was ignored. Maybe leave it that way, turn Load to void.

@@ +100,5 @@
> +{
> +  MonitorAutoLock mon(mMonitor);
> +  while (mIsLoading) {
> +    mon.Wait();
> +  }

if (mIsLoading) should be enough

@@ +111,5 @@
> +  MonitorAutoLock mon(mMonitor);
> +
> +  while (mIsLoading) {
> +    earlyExit = true;
> +    mon.Wait();

This logic is wrong.  When Load() is called sooner then the actual load is done, it will just early exit.  When called after we have loaded, it will reload (block again).  That is not what you want.

Rather have a flag mIsLoaded (or just mLoaded).  False by default, here you will return when mLoaded is true.  In SyncLoad you set it to true (under the monitor being locked indeed).

EnsureDictIsLoaded() will then do: |if (!mLoaded) { mon.Wait(); }|

@@ +132,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult mozPersonalDictionary::SyncLoad()
> +{

assert you are not on the main thread here

@@ +137,2 @@
>    //FIXME Deinst  -- get dictionary name from prefs;
>    nsresult res;

please change this to |rv|

@@ +145,1 @@
>    res = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(theFile));

does this work off the main thread?  I once had problems with that, you may need to grab in on the main thread and pass via a member.  There is no IO caused by NS_GetSpecialDirectory.

@@ +145,2 @@
>    res = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(theFile));
> +  if(NS_FAILED(res)) {

please space between if and (

@@ +145,4 @@
>    res = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(theFile));
> +  if(NS_FAILED(res)) {
> +    mon.Notify();
> +    return res;

since doing mon.Notify() before every early return, please have a "SyncLoadInternal" method that does the actual stuff and you will call somehow like this:

..::SyncLoad()
{
  MonitorAutoLock mon(mMonitor);
  ...
  rv = SyncLoadInternal();
  mon.Notify();
  ...
  return rv;
}

@@ +317,5 @@
>  
>  /* void IgnoreWord (in wstring word); */
>  NS_IMETHODIMP mozPersonalDictionary::IgnoreWord(const char16_t *aWord)
>  {
> +  EnsureDictIsLoaded();

you don't need it here, I think.

@@ +357,5 @@
>  /* void observe (in nsISupports aSubject, in string aTopic, in wstring aData); */
>  NS_IMETHODIMP mozPersonalDictionary::Observe(nsISupports *aSubject, const char *aTopic, const char16_t *aData)
>  {
>    if (!nsCRT::strcmp(aTopic, "profile-do-change")) {
> +    Load(); //load automatically clears out the existing dictionary table

So, this is where we start the load for the first time if Init() has not been called so far on this object, right?

To have a better startup performance, I suggest to do this from "sessionstore-windows-restored" topic.

::: extensions/spellcheck/src/mozPersonalDictionary.h
@@ +51,5 @@
> +private:
> +  bool mIsLoading;
> +  Monitor mMonitor;
> +
> +  void EnsureDictIsLoaded();

WaitForLoad() is IMO a better name, but up to you

@@ +54,5 @@
> +
> +  void EnsureDictIsLoaded();
> +  nsresult SyncLoad();
> +
> +  friend mozPersonalDictionaryLoader;

please add comments what each member does, how changes etc.

Comment 11

[Roberto Agostino Vitillo (:rvitillo)]

(In reply to Honza Bambas (:mayhemer) from comment #10)
>> @@ +62,5 @@
> > +    return mDict->SyncLoad();
> > +  }
> > +
> > +private:
> > +  mozPersonalDictionary *mDict;
> 
> do a hard ref (nsRefPtr)

If I use a nsRefPtr for mozPersonalDictionary, I hit the following assertion:
Hit MOZ_CRASH(mozPersonalDictionary not thread-safe) at /Users/vitillo/sandbox/mozilla-central/extensions/spellcheck/src/mozPersonalDictionary.cpp:40

Any thoughts on how to avoid that error without using NS_DECL_THREADSAFE_ISUPPORTS in mozPersonalDictionary?

> @@ +89,5 @@
> >    NS_ENSURE_SUCCESS(rv, rv);
> >    rv = svc->AddObserver(this, "profile-before-change", true);
> >    NS_ENSURE_SUCCESS(rv, rv);
> >  
> > +  rv = Load();
> 
> The name should be StartLoad() or Preload()

Load() is part of the mozIPersonalDictionary interface.

> When is Init() actually invoked?

It’s invoked when HTMLInputElement receives a focus event for the first time.

> @@ +90,5 @@
> >    rv = svc->AddObserver(this, "profile-before-change", true);
> >    NS_ENSURE_SUCCESS(rv, rv);
> >  
> > +  rv = Load();
> > +  NS_ENSURE_SUCCESS(rv, rv);
> 
> Hmm.. before that the result was ignored. Maybe leave it that way, turn Load
> to void.

Load() is part of the mozIPersonalDictionary interface.

> @@ +111,5 @@
> > +  MonitorAutoLock mon(mMonitor);
> > +
> > +  while (mIsLoading) {
> > +    earlyExit = true;
> > +    mon.Wait();
> 
> This logic is wrong.  When Load() is called sooner then the actual load is
> done, it will just early exit.  When called after we have loaded, it will
> reload (block again).  That is not what you want.

When Load is called for the first time, it will not early exit since mIsLoading is set to false. If Load is called a second time, and the asynchronous load performed by the runnable has not yet completed, the Load will wait for completion. The logic effectively serializes multiple calls to Load; i.e. I wanted to keep the same behavior as before.

> Rather have a flag mIsLoaded (or just mLoaded).  False by default, here you
> will return when mLoaded is true.  In SyncLoad you set it to true (under the
> monitor being locked indeed).

Ok, it doesn’t make sense to reload multiple times the same dictionary, even if a user of mozIPersonalDictionary calls multiple times the Load method.
 
> @@ +145,1 @@
> >    res = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(theFile));
> 
> does this work off the main thread?  I once had problems with that, you may
> need to grab in on the main thread and pass via a member.  There is no IO
> caused by NS_GetSpecialDirectory.

Yes it works.

> @@ +357,5 @@
> >  /* void observe (in nsISupports aSubject, in string aTopic, in wstring aData); */
> >  NS_IMETHODIMP mozPersonalDictionary::Observe(nsISupports *aSubject, const char *aTopic, const char16_t *aData)
> >  {
> >    if (!nsCRT::strcmp(aTopic, "profile-do-change")) {
> > +    Load(); //load automatically clears out the existing dictionary table
> 
> So, this is where we start the load for the first time if Init() has not
> been called so far on this object, right?

Init() adds the observer so it will always be called before Observe.

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #11)
> (In reply to Honza Bambas (:mayhemer) from comment #10)
> >> @@ +62,5 @@
> > > +    return mDict->SyncLoad();
> > > +  }
> > > +
> > > +private:
> > > +  mozPersonalDictionary *mDict;
> > 
> > do a hard ref (nsRefPtr)
> 
> If I use a nsRefPtr for mozPersonalDictionary, I hit the following assertion:
> Hit MOZ_CRASH(mozPersonalDictionary not thread-safe) at
> /Users/vitillo/sandbox/mozilla-central/extensions/spellcheck/src/
> mozPersonalDictionary.cpp:40

Release it from the refptr back on the main thread.

> 
> Any thoughts on how to avoid that error without using
> NS_DECL_THREADSAFE_ISUPPORTS in mozPersonalDictionary?
> 
> > @@ +89,5 @@
> > >    NS_ENSURE_SUCCESS(rv, rv);
> > >    rv = svc->AddObserver(this, "profile-before-change", true);
> > >    NS_ENSURE_SUCCESS(rv, rv);
> > >  
> > > +  rv = Load();
> > 
> > The name should be StartLoad() or Preload()
> 
> Load() is part of the mozIPersonalDictionary interface.

Hmm.. but the meaning has changed.  Have to think.

> 
> > When is Init() actually invoked?
> 
> It’s invoked when HTMLInputElement receives a focus event for the first time.

That's good.

> > @@ +111,5 @@
> > > +  MonitorAutoLock mon(mMonitor);
> > > +
> > > +  while (mIsLoading) {
> > > +    earlyExit = true;
> > > +    mon.Wait();
> > 
> > This logic is wrong.  When Load() is called sooner then the actual load is
> > done, it will just early exit.  When called after we have loaded, it will
> > reload (block again).  That is not what you want.
> 
> When Load is called for the first time, it will not early exit since
> mIsLoading is set to false. If Load is called a second time, and the
> asynchronous load performed by the runnable has not yet completed, the Load
> will wait for completion. The logic effectively serializes multiple calls to
> Load; i.e. I wanted to keep the same behavior as before.

That is wasting.  Why should we keep such a logic?  Feel free to change it, that seems to me wrong.  But I have to check it again.

> 
> > Rather have a flag mIsLoaded (or just mLoaded).  False by default, here you
> > will return when mLoaded is true.  In SyncLoad you set it to true (under the
> > monitor being locked indeed).
> 
> Ok, it doesn’t make sense to reload multiple times the same dictionary, even
> if a user of mozIPersonalDictionary calls multiple times the Load method.

Yep, that is the point.

>  
> > @@ +145,1 @@
> > >    res = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(theFile));
> > 
> > does this work off the main thread?  I once had problems with that, you may
> > need to grab in on the main thread and pass via a member.  There is no IO
> > caused by NS_GetSpecialDirectory.
> 
> Yes it works.

Interesting :)

> 
> > @@ +357,5 @@
> > >  /* void observe (in nsISupports aSubject, in string aTopic, in wstring aData); */
> > >  NS_IMETHODIMP mozPersonalDictionary::Observe(nsISupports *aSubject, const char *aTopic, const char16_t *aData)
> > >  {
> > >    if (!nsCRT::strcmp(aTopic, "profile-do-change")) {
> > > +    Load(); //load automatically clears out the existing dictionary table
> > 
> > So, this is where we start the load for the first time if Init() has not
> > been called so far on this object, right?
> 
> Init() adds the observer so it will always be called before Observe.

Also doesn't make much sense.


I have to take a look at this patch again.  Feel free to update parts that are not disputable and ask for another round of feedback.  Tho, I will get to it no sooner then the next week.

Comment 13

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: User dictionary (persdict.dat) read on main thread, v4

Comment 14

[(no longer active)]

Comment on attachment 8404114 [details] [diff] [review]
User dictionary (persdict.dat) read on main thread, v4

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

Some drive-by comments.

::: extensions/spellcheck/src/mozPersonalDictionary.cpp
@@ +49,5 @@
>  
>  NS_IMPL_CYCLE_COLLECTION_1(mozPersonalDictionary, mEncoder)
>  
>  mozPersonalDictionary::mozPersonalDictionary()
> + : mDirty(false), mIsLoaded(false), mMonitor("mozPersonalDictionary::mMonitor")

Please init one variable per line.

@@ +99,5 @@
> +
> +  nsCOMPtr<nsIRunnable> runnable = new mozPersonalDictionaryLoader(this);
> +
> +  rv = target->Dispatch(runnable, NS_DISPATCH_NORMAL);
> +  NS_ENSURE_SUCCESS(rv, rv);

Just return rv.

@@ +121,5 @@
> +
> +  return rv;
> +}
> +
> +nsresult mozPersonalDictionary::SyncLoadInternal() {

Please MOZ_ASSERT the expected thread here.  Also, nit: please put the brace on its own line.

::: extensions/spellcheck/src/mozPersonalDictionary.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ /* This Source Code Form is subject to the terms of the Mozilla Public * License, v. 2.0. If a copy of the MPL was not distributed with this

Please revert this hunk.

@@ +53,5 @@
> +    }
> +
> +    NS_IMETHOD Run()
> +    {
> +      if (!NS_IsMainThread()) {

Please MOZ_ASSERT the thread here.  Also, please move this class to the cpp file in order to avoid having to include a bunch of stuff in the header.

@@ +55,5 @@
> +    NS_IMETHOD Run()
> +    {
> +      if (!NS_IsMainThread()) {
> +	mDict->SyncLoad();
> +	NS_DispatchToMainThread(this);

What's the point of dispatching this runnable back to the main thread here?

@@ +66,5 @@
> +    nsRefPtr<mozPersonalDictionary> mDict;
> +  };
> +
> +  bool mIsLoaded; /* true if the dictionary has been loaded from disk */
> +  mozilla::Monitor mMonitor;

Please group these member variables together with the rest of them above.

Comment 15

[Roberto Agostino Vitillo (:rvitillo)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #14)

> @@ +53,5 @@
> > +    }
> > +
> > +    NS_IMETHOD Run()
> > +    {
> > +      if (!NS_IsMainThread()) {
> 
> Please MOZ_ASSERT the thread here.  Also, please move this class to the cpp
> file in order to avoid having to include a bunch of stuff in the header.
> 
> @@ +55,5 @@
> > +    NS_IMETHOD Run()
> > +    {
> > +      if (!NS_IsMainThread()) {
> > +	mDict->SyncLoad();
> > +	NS_DispatchToMainThread(this);
> 
> What's the point of dispatching this runnable back to the main thread here?

The refptr needs to be released on the main-thread to avoid 
"MOZ_CRASH(mozPersonalDictionary not thread-safe)", see comment 12.

Comment 16

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: User dictionary (persdict.dat) read on main thread, v5

Comment 17

[(no longer active)]

(In reply to comment #15)
> > What's the point of dispatching this runnable back to the main thread here?
> 
> The refptr needs to be released on the main-thread to avoid 
> "MOZ_CRASH(mozPersonalDictionary not thread-safe)", see comment 12.

Oh right.  Sorry I missed that!  Please add a comment there.

Comment 18

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: User dictionary (persdict.dat) read on main thread, v5

Comment 19

[Honza Bambas (:mayhemer)]

Comment on attachment 8405382 [details] [diff] [review]
User dictionary (persdict.dat) read on main thread, v5

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

nice!

r=honzab

::: extensions/spellcheck/src/mozPersonalDictionary.cpp
@@ +65,5 @@
> +      NS_DispatchToMainThread(this);
> +    }
> +
> +    return NS_OK;
> +  }

nit: as I'm thinking of it, the mozPersonalDictionary could impl nsIRunnable directly.  up to you to leave this helper class or move to the dictionary.

@@ +80,5 @@
>  }
>  
>  mozPersonalDictionary::~mozPersonalDictionary()
>  {
> +  WaitForLoad();

still?

@@ +106,5 @@
> +  mozilla::MonitorAutoLock mon(mMonitor);
> +
> +  if (!mIsLoaded) {
> +    mon.Wait();
> +  }

optimization: check the flag |if (mIsLoaded) return;| before entering the monitor.  it's actually thread safe and will save some resources and performance.

@@ +137,5 @@
> +  if (mIsLoaded) {
> +    return NS_OK;
> +  }
> +
> +  mIsLoaded = true;

Do after SyncLoadInternal() returns to make above suggestion work

@@ +141,5 @@
> +  mIsLoaded = true;
> +  nsresult rv = SyncLoadInternal();
> +  mon.Notify();
> +
> +  return rv;

returning a result has no meaning, make SyncLoad* void or carry the result in a member and fail callers when it's NS_FAILED().  But I personally would not bother.  In ideal world it would be good to log to the console about load of user dict failure, but I really would not bother...

@@ +361,5 @@
>  NS_IMETHODIMP mozPersonalDictionary::Observe(nsISupports *aSubject, const char *aTopic, const char16_t *aData)
>  {
>    if (!nsCRT::strcmp(aTopic, "profile-do-change")) {
> +    mIsLoaded = false;
> +    Load(); //load automatically clears out the existing dictionary table

To be safe (since transport server provides a pool of thread, not just a single thread), do WaitForLoad() before dropping the flag.  comments appreciated.

nit: keep space between // and load

::: extensions/spellcheck/src/mozPersonalDictionary.h
@@ +52,5 @@
> +
> +private:
> +  void WaitForLoad(); /* wait for the asynchronous load of the dictionary to be completed */
> +  nsresult SyncLoad(); /* perform a synchronous load of the dictionary from disk */
> +  nsresult SyncLoadInternal();

feel free to put the comment before methods and break the (bad) style of this header.

some comment on the internal method may be useful to.

Comment 20

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: User dictionary (persdict.dat) read on main thread, v6

Comment 21

[Honza Bambas (:mayhemer)]

Comment on attachment 8408329 [details] [diff] [review]
User dictionary (persdict.dat) read on main thread, v6

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

want to ask for r?

::: extensions/spellcheck/src/mozPersonalDictionary.h
@@ +59,5 @@
> +private:
> +  /* wait for the asynchronous load of the dictionary to be completed */
> +  void WaitForLoad();
> +
> +  /* enter the monitor before starting a synchronous load */

maybe note this is called on a non-main thread

Comment 22

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: User dictionary (persdict.dat) read on main thread, v6

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea78684e8716

Comment 24

[Carsten Book [:Tomcat]]

sorry had tp backed this out for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=38241308&tree=Mozilla-Inbound - could you run a try server run before you request checkin-needed next time thx :) ?

Comment 25

[Ryan VanderMeulen [:RyanVM]]

This broke Android and B2G badly too with crashes like the below:
https://tbpl.mozilla.org/php/getParsedLog.php?id=38242702&tree=Mozilla-Inbound

Full Try run please :)

Comment 26

[Honza Bambas (:mayhemer)]

(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #11)
> (In reply to Honza Bambas (:mayhemer) from comment #10)
> > @@ +145,1 @@
> > >    res = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(theFile));
> > 
> > does this work off the main thread?  I once had problems with that, you may
> > need to grab in on the main thread and pass via a member.  There is no IO
> > caused by NS_GetSpecialDirectory.
> 
> Yes it works.

I told you...

Comment 27

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: User dictionary (persdict.dat) read on main thread, v7

https://tbpl.mozilla.org/?tree=Try&rev=b864151bcf3c

Comment 28

[Honza Bambas (:mayhemer)]

Comment on attachment 8410954 [details] [diff] [review]
User dictionary (persdict.dat) read on main thread, v7

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

r=honzab

Push to try before landing this time!  Thanks.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Those e10s mochitest failures sure look scary. Do you know for sure your patch isn't the cause? Because I don't recall any recent TBPL bustage that looks like those.

Comment 30

[Honza Bambas (:mayhemer)]

:rvitillo, if you don't have level 1 commit access (to push to try), ask for it, or ask someone to push to try for you.  But please do that, and expose (or let be exposed) the link to the run here before you ask for checkin-needed!  Otherwise you will again unnecessarily ruin the tree and disturb other peoples work.  This is important.  Thanks.

Comment 31

[Roberto Agostino Vitillo (:rvitillo)]

Honza, I actually pushed to try as you can see in comment 27. I thought the e10s mochitest failures were intermittent ones so I will have to investigate further. Sorry about that.

Comment 32

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: User dictionary (persdict.dat) read on main thread, v8

When NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, ...) was failing the boolean flag mIsLoaded was never set causing WaitForLoad() to wait indefinitely.

In one try build I could see one particular failure (Android 4.0 Debug, M7) which didn't appear in a second one. Honza, do you think that the failure might be related to this patch?

https://tbpl.mozilla.org/?tree=Try&rev=9a57052ed1e0
https://tbpl.mozilla.org/?tree=Try&rev=becdf0961956

Comment 33

[Honza Bambas (:mayhemer)]

(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #32)
> Created attachment 8415148 [details] [diff] [review]
> User dictionary (persdict.dat) read on main thread, v8
> 
> When NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, ...) was failing the
> boolean flag mIsLoaded was never set causing WaitForLoad() to wait
> indefinitely.

Yep, that was wrong.

> 
> In one try build I could see one particular failure (Android 4.0 Debug, M7)
> which didn't appear in a second one. Honza, do you think that the failure
> might be related to this patch?
> 
> https://tbpl.mozilla.org/?tree=Try&rev=9a57052ed1e0
> https://tbpl.mozilla.org/?tree=Try&rev=becdf0961956

Could be.  I think it was a hang crash (watchdog), but hard to say.  Definitely should now be fixed.

Comment 34

[Honza Bambas (:mayhemer)]

Comment on attachment 8415148 [details] [diff] [review]
User dictionary (persdict.dat) read on main thread, v8

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

nit: in a new code, instead NS_ENSURE_SUCCESS the preferred way (despite I'm personally against!!) is 

if (NS_WARN_IF(NS_FAILED(rv))) { 
  return rv;
}

::: extensions/spellcheck/src/mozPersonalDictionary.cpp
@@ +112,5 @@
> +    mon.Wait();
> +  }
> +}
> +
> +nsresult mozPersonalDictionary::TryLoad()

rather LoadInternal()

@@ +122,5 @@
> +    return NS_OK;
> +  }
> +
> +  nsCOMPtr<nsIEventTarget> target = do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);

get the target just before dispatch

@@ +136,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsIRunnable> runnable = new mozPersonalDictionaryLoader(this);
> +  rv = target->Dispatch(runnable, NS_DISPATCH_NORMAL);
> +  return rv;

either (preferred)
NS_ENSURE_SUCCESS(rv, rv)
return NS_OK

or just
return target->Dispatch(...

::: extensions/spellcheck/src/mozPersonalDictionary.h
@@ +63,5 @@
> +  /* enter the monitor before starting a synchronous load off the main-thread */
> +  void SyncLoad();
> +
> +  /* try to start an asynchrounous load of the dictionary from the main-thread */
> +  nsresult TryLoad();

some more detailed comment is needed, e.g. say how it plays with the object state etc..

Comment 35

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: User dictionary (persdict.dat) read on main thread, v9

https://tbpl.mozilla.org/?tree=Try&rev=1e0b670a35de

Comment 36

[Honza Bambas (:mayhemer)]

Few words about what the patch does new?

Comment 37

[Roberto Agostino Vitillo (:rvitillo)]

I just applied your last comments and pushed it to try.

Comment 38

[Honza Bambas (:mayhemer)]

Comment on attachment 8418097 [details] [diff] [review]
User dictionary (persdict.dat) read on main thread, v9

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

::: extensions/spellcheck/src/mozPersonalDictionary.cpp
@@ +147,5 @@
> +  }
> +
> +  nsCOMPtr<nsIRunnable> runnable = new mozPersonalDictionaryLoader(this);
> +  rv = target->Dispatch(runnable, NS_DISPATCH_NORMAL);
> +  NS_WARN_IF(NS_FAILED(rv));

if (NS_WARN_IF(NS_FAILED(rv)))
  return rv;

return NS_OK;

@@ +376,5 @@
>  /* void observe (in nsISupports aSubject, in string aTopic, in wstring aData); */
>  NS_IMETHODIMP mozPersonalDictionary::Observe(nsISupports *aSubject, const char *aTopic, const char16_t *aData)
>  {
>    if (!nsCRT::strcmp(aTopic, "profile-do-change")) {
> +    WaitForLoad();

could it happen that :Load() has never been called before this code is executed?  If so, this will stuck forever.  Maybe register to "profile-do-change" in LoadInternal() then?

Comment 39

[Roberto Agostino Vitillo (:rvitillo)]

(In reply to Honza Bambas (:mayhemer) from comment #38)

> @@ +376,5 @@
> >  /* void observe (in nsISupports aSubject, in string aTopic, in wstring aData); */
> >  NS_IMETHODIMP mozPersonalDictionary::Observe(nsISupports *aSubject, const char *aTopic, const char16_t *aData)
> >  {
> >    if (!nsCRT::strcmp(aTopic, "profile-do-change")) {
> > +    WaitForLoad();
> 
> could it happen that :Load() has never been called before this code is
> executed?  If so, this will stuck forever.  Maybe register to
> "profile-do-change" in LoadInternal() then?

The observer is registered in Init() which calls Load and in turn LoadInternal() so it's safe to leave it like that.

Comment 40

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: User dictionary (persdict.dat) read on main thread, v10

Changed NS_WARN_IF(... to if (NS_WARN_IF...

Comment 41

[Honza Bambas (:mayhemer)]

Comment on attachment 8418689 [details] [diff] [review]
User dictionary (persdict.dat) read on main thread, v10

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

r=honzab, go forward!

::: extensions/spellcheck/src/mozPersonalDictionary.cpp
@@ +381,5 @@
>  {
>    if (!nsCRT::strcmp(aTopic, "profile-do-change")) {
> +    WaitForLoad();
> +    mIsLoaded = false;
> +    Load(); // load automatically clears out the existing dictionary table

please add here your comment from comment 39

Comment 42

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: User dictionary (persdict.dat) read on main thread, v10

Comment 43

[Roberto Agostino Vitillo (:rvitillo)]

try build: https://tbpl.mozilla.org/?tree=Try&rev=1c8a7840b4b5

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9120071e807

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e9120071e807