User dictionary (persdict.dat) read on main thread

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: vladan, Assigned: rvitillo)

Tracking

(Blocks 1 bug, {main-thread-io})

unspecified
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:P2])

Attachments

(2 attachments, 13 obsolete attachments)

9.40 KB, text/plain
Details
12.37 KB, patch
Details | Diff | Splinter Review
Posted file Stack trace
We're reading the user's personal dictionary on the main thread shortly after startup. See attached stack
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...
Blocks: 572459
Assignee: nobody → rvitillo
Attachment #8400059 - Flags: review?(ehsan)
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?
Attachment #8400059 - Flags: review?(ehsan) → review-
Attachment #8400059 - Attachment is obsolete: true
Attachment #8401193 - Flags: review?(ehsan)
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.
Attachment #8401193 - Flags: review?(ehsan) → review-
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?
Monday sounds good, thanks.
Honza, is this what you had in mind?
Attachment #8401193 - Attachment is obsolete: true
Attachment #8402675 - Attachment is obsolete: true
Attachment #8402678 - Flags: review?(honzab.moz)
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.
Attachment #8402678 - Flags: review?(honzab.moz) → feedback+
(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.
Flags: needinfo?(honzab.moz)
(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.
Flags: needinfo?(honzab.moz)
Attachment #8402678 - Attachment is obsolete: true
Attachment #8404114 - Flags: review?(honzab.moz)
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.
(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.
Attachment #8404114 - Attachment is obsolete: true
Attachment #8404114 - Flags: review?(honzab.moz)
Attachment #8404812 - Flags: review?(honzab.moz)
(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.
Attachment #8404812 - Attachment is obsolete: true
Attachment #8404812 - Flags: review?(honzab.moz)
Attachment #8405382 - Flags: review?(honzab.moz)
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.
Attachment #8405382 - Flags: review?(honzab.moz) → review+
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
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 :) ?
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 :)
(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 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.
Attachment #8410954 - Flags: review?(honzab.moz) → review+
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.
Keywords: checkin-needed
: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.
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.
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
Attachment #8410954 - Attachment is obsolete: true
Attachment #8415148 - Flags: feedback?(honzab.moz)
(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 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..
Attachment #8415148 - Flags: feedback?(honzab.moz) → feedback+
Few words about what the patch does new?
I just applied your last comments and pushed it to try.
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?
(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.
Changed NS_WARN_IF(... to if (NS_WARN_IF...
Attachment #8418097 - Attachment is obsolete: true
Attachment #8418097 - Flags: review?(honzab.moz)
Attachment #8418689 - Flags: review?(honzab.moz)
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
Attachment #8418689 - Flags: review?(honzab.moz) → review+
https://hg.mozilla.org/mozilla-central/rev/e9120071e807
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.