nsEditorSpellCheck::SaveDefaultDictionary fails in content processes trying to set spellchecker.dictionary pref

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

It's trying to save the value of GetCurrentDictionary().  I have no clue how bad this failure to set the pref is.  If it's bad, this needs to become a first-class remoted API.  That's a bit tricky; how are races to set the pref resolved?  Would chrome try to set it too?  Or is the value set always the same?

If the failure-to-set isn't going to cause problems, can setting the pref be removed?

Really, I'm just fishing for advice from someone in the know :).

Setting to block bug 615386 mainly for tracking purposes, but it doesn't hard block.  The assertion can be temporarily weakened while this is resolved.
Spoke with Ehsan on IRC.  My understanding of the problem per that discussion is

 (1) the dictionary is initially selected on the basis of this pref
 (2) the user can change the dictionary (through some FF UI?)
 (2a) (the pref can change otherwise, but this isn't honored)
 (3) when the dictionary is uninitialized, because of (2), the currently selected dictionary is saved to the pref, so that the user-selected one is loaded the next time

Is that approximately correct?

If so, then we do need to support saving this preference (somehow).  This seems fairly complicated to me in the face of multiple content processes etc.  I also don't even know if fennec supports changing the dictionary, or uses a dictionary.

Ehsan also noted that this code could, and perhaps should, save the value of the pref when the dictionary is initially loaded, and then only write the pref if it changed.  That's not really going to win any perf, but would work around this issue for the time being without changing existing behavior.

Thoughts?
Oops, meant to CC mfinkle but mid-aired with ehsan.
Hmmmmm this is a rather more general problem.  The reason we try to write the pref the first time is because of

NS_IMETHODIMP
nsChromeRegistryContent::GetSelectedLocale(const nsACString& aPackage,
                                           nsACString& aLocale)
{
  CONTENT_NOT_IMPLEMENTED();
}

(see bug 576507), which gets us to

  if (NS_SUCCEEDED(rv) && !dictName.IsEmpty()) {
    rv = SetCurrentDictionary(dictName.get());

    // fall back to "en-US" if the current locale doesn't have a dictionary.
    if (NS_FAILED(rv)) {
      rv = SetCurrentDictionary(NS_LITERAL_STRING("en-US").get());
    }

with rv having an error value.  Is there an important reason the logic here is

 if (locale)
   try locale
   if (failed)
     try en-US
 else
   (look through list of dictionaries)

instead of

 if (locale)
   try local
 else
   try en-US
 else
  (look through list of dictionaries)

?  That's a possible workaround for the more general problem.

It's beginning to look like we're going to need the locale in the content process.  CC'ing bsmedberg for thoughts on this case.
Er, 

 if (locale)
   try local
 else if (!local or local failed)
   try en-US
 else
  (look through list of dictionaries)
I don't know if this patch is idiotic or not.  Ehsan, I also don't know if you're the right feedback-er, please let me know.
Assignee: nobody → jones.chris.g
Attachment #496331 - Flags: feedback?(ehsan)
This patch will result in the same dictionary being chosen as the code on m-c tip.  The difference is, the dictionary chosen isn't written to the pref if the dictionary never changed after init.
Attachment #496338 - Flags: feedback?(ehsan)

Comment 7

9 years ago
(In reply to comment #1)
> Spoke with Ehsan on IRC.  My understanding of the problem per that discussion
> is
> 
>  (1) the dictionary is initially selected on the basis of this pref
>  (2) the user can change the dictionary (through some FF UI?)
>  (2a) (the pref can change otherwise, but this isn't honored)
>  (3) when the dictionary is uninitialized, because of (2), the currently
> selected dictionary is saved to the pref, so that the user-selected one is
> loaded the next time
> 
> Is that approximately correct?

Yes.  The UI in question in comment 2 is the spell checker context menu.

> If so, then we do need to support saving this preference (somehow).  This seems
> fairly complicated to me in the face of multiple content processes etc.  I also
> don't even know if fennec supports changing the dictionary, or uses a
> dictionary.
> 
> Ehsan also noted that this code could, and perhaps should, save the value of
> the pref when the dictionary is initially loaded, and then only write the pref
> if it changed.  That's not really going to win any perf, but would work around
> this issue for the time being without changing existing behavior.

Looks like a good option...

Comment 8

9 years ago
Comment on attachment 496331 [details] [diff] [review]
Try en-US before enumerating dictionaries

This will cause us not to pick up the dictionary after it's been installed for the first time in en-US builds.
Attachment #496331 - Flags: feedback?(ehsan) → review-
(In reply to comment #8)
> Comment on attachment 496331 [details] [diff] [review]
> Try en-US before enumerating dictionaries
> 
> This will cause us not to pick up the dictionary after it's been installed for
> the first time in en-US builds.

"It" being firefox?

Comment 10

9 years ago
Comment on attachment 496338 [details] [diff] [review]
Don't save the dictionary to a pref if it didn't change

>+  {
>+    PRUnichar *initialDict = nsnull;
>+    rv = GetCurrentDictionary(&initialDict);
>+    if (NS_SUCCEEDED(rv) && initialDict) {
>+      mInitialDictionary.Adopt(initialDict);
>+      initialDict = nsnull;
>+    } else {
>+      NS_WARNING("Failed to select dictionary!");
>+    }
>+
>+    NS_ABORT_IF_FALSE(!initialDict, "About to leak memory");

Isn't it better to not leak here, instead of aborting if we do?  You should just free initialDict if it's not null and NS_FAILED(rv).  Actually, why don't you just use getter_Copies here?

In fact, can't we just set a boolean flag in SetCurrentDictionary to save us all this blood and sweat?
Attachment #496338 - Flags: feedback?(ehsan) → review-
(In reply to comment #10)
> Comment on attachment 496338 [details] [diff] [review]
> Don't save the dictionary to a pref if it didn't change
> 
> >+  {
> >+    PRUnichar *initialDict = nsnull;
> >+    rv = GetCurrentDictionary(&initialDict);
> >+    if (NS_SUCCEEDED(rv) && initialDict) {
> >+      mInitialDictionary.Adopt(initialDict);
> >+      initialDict = nsnull;
> >+    } else {
> >+      NS_WARNING("Failed to select dictionary!");
> >+    }
> >+
> >+    NS_ABORT_IF_FALSE(!initialDict, "About to leak memory");
> 
> Isn't it better to not leak here, instead of aborting if we do?  You should
> just free initialDict if it's not null and NS_FAILED(rv).  Actually, why don't
> you just use getter_Copies here?
> 

This code doesn't leak.  It has mInitialDictionary Adopt() the raw-allocated intialDict if it's nonnull.  But if someone comes along in the future and changes this code incorrectly, that assertion will catch accidentally-added leaks.

(I'd like to change the GetCurrentDictionary() API, but that seems like more project than I want to take on for reftest-ipc ;) .)

> In fact, can't we just set a boolean flag in SetCurrentDictionary to save us
> all this blood and sweat?

To do what?

Comment 12

9 years ago
(In reply to comment #9)
> (In reply to comment #8)
> > Comment on attachment 496331 [details] [diff] [review]
> > Try en-US before enumerating dictionaries
> > 
> > This will cause us not to pick up the dictionary after it's been installed for
> > the first time in en-US builds.
> 
> "It" being firefox?

No, a new dictionary.
(In reply to comment #12)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Comment on attachment 496331 [details] [diff] [review] [details]
> > > Try en-US before enumerating dictionaries
> > > 
> > > This will cause us not to pick up the dictionary after it's been installed for
> > > the first time in en-US builds.
> > 
> > "It" being firefox?
> 
> No, a new dictionary.

Does installing a new dictionary (through firefox, or externally?) clear the old pref?

Comment 14

9 years ago
(In reply to comment #11)
> (In reply to comment #10)
> > Comment on attachment 496338 [details] [diff] [review]
> > Don't save the dictionary to a pref if it didn't change
> > 
> > >+  {
> > >+    PRUnichar *initialDict = nsnull;
> > >+    rv = GetCurrentDictionary(&initialDict);
> > >+    if (NS_SUCCEEDED(rv) && initialDict) {
> > >+      mInitialDictionary.Adopt(initialDict);
> > >+      initialDict = nsnull;
> > >+    } else {
> > >+      NS_WARNING("Failed to select dictionary!");
> > >+    }
> > >+
> > >+    NS_ABORT_IF_FALSE(!initialDict, "About to leak memory");
> > 
> > Isn't it better to not leak here, instead of aborting if we do?  You should
> > just free initialDict if it's not null and NS_FAILED(rv).  Actually, why don't
> > you just use getter_Copies here?
> > 
> 
> This code doesn't leak.  It has mInitialDictionary Adopt() the raw-allocated
> intialDict if it's nonnull.  But if someone comes along in the future and
> changes this code incorrectly, that assertion will catch accidentally-added
> leaks.

It does if rv is a failure code but the function sets the value of its outparam (I know, that would violate XPCOM rules and all, but stil...)  But I think you should use getter_Copies anyway, as I said above.

> (I'd like to change the GetCurrentDictionary() API, but that seems like more
> project than I want to take on for reftest-ipc ;) .)

Fair enough!

> > In fact, can't we just set a boolean flag in SetCurrentDictionary to save us
> > all this blood and sweat?
> 
> To do what?

To determine whether we need to set the pref or not...  Although maybe that won't work...

Comment 15

9 years ago
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #9)
> > > (In reply to comment #8)
> > > > Comment on attachment 496331 [details] [diff] [review] [details]
> > > > Try en-US before enumerating dictionaries
> > > > 
> > > > This will cause us not to pick up the dictionary after it's been installed for
> > > > the first time in en-US builds.
> > > 
> > > "It" being firefox?
> > 
> > No, a new dictionary.
> 
> Does installing a new dictionary (through firefox, or externally?) clear the
> old pref?

No (well, I don't think so).  If you're on a xx-YY system and an en-US firefox, installing an xx-YY dictionary and restarting Firefox would lead into en-US being picked up again.  The key here is dictName being empty or not.
(In reply to comment #14)
> (In reply to comment #11)
> > This code doesn't leak.  It has mInitialDictionary Adopt() the raw-allocated
> > intialDict if it's nonnull.  But if someone comes along in the future and
> > changes this code incorrectly, that assertion will catch accidentally-added
> > leaks.
> 
> It does if rv is a failure code but the function sets the value of its outparam
> (I know, that would violate XPCOM rules and all, but stil...)  But I think you
> should use getter_Copies anyway, as I said above.
> 

Oh, good call.

> > > In fact, can't we just set a boolean flag in SetCurrentDictionary to save us
> > > all this blood and sweat?
> > 
> > To do what?
> 
> To determine whether we need to set the pref or not...  Although maybe that
> won't work...

Oh, I see: add an internal SetCurrentDictionary() with a flag like |bool andSetAsDefault|?  Hmm, I'm not sure that would work; in Init(), we'd use |false| (IIUC), and the public SetCurrentDictionary() would need to use the |false| variant too, to avoid changing the API contract.

(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (In reply to comment #9)
> > > > (In reply to comment #8)
> > > > > Comment on attachment 496331 [details] [diff] [review] [details] [details]
> > > > > Try en-US before enumerating dictionaries
> > > > > 
> > > > > This will cause us not to pick up the dictionary after it's been installed for
> > > > > the first time in en-US builds.
> > > > 
> > > > "It" being firefox?
> > > 
> > > No, a new dictionary.
> > 
> > Does installing a new dictionary (through firefox, or externally?) clear the
> > old pref?
> 
> No (well, I don't think so).  If you're on a xx-YY system and an en-US firefox,
> installing an xx-YY dictionary and restarting Firefox would lead into en-US
> being picked up again.  The key here is dictName being empty or not.

But on exiting, Firefox would write spellchecker.dictionary="en-US", and then after restarting, Init() would read in spellchecker.dictionary="en-US" and skip the dictionary-scanning logic.  (Unless I'm missing something.)

Comment 17

9 years ago
(In reply to comment #16)
> > > > In fact, can't we just set a boolean flag in SetCurrentDictionary to save us
> > > > all this blood and sweat?
> > > 
> > > To do what?
> > 
> > To determine whether we need to set the pref or not...  Although maybe that
> > won't work...
> 
> Oh, I see: add an internal SetCurrentDictionary() with a flag like |bool
> andSetAsDefault|?  Hmm, I'm not sure that would work; in Init(), we'd use
> |false| (IIUC), and the public SetCurrentDictionary() would need to use the
> |false| variant too, to avoid changing the API contract.

You know what?  This is not worth the complexity at all, I retract that comment!

> (In reply to comment #15)
> > (In reply to comment #13)
> > > (In reply to comment #12)
> > > > (In reply to comment #9)
> > > > > (In reply to comment #8)
> > > > > > Comment on attachment 496331 [details] [diff] [review] [details] [details] [details]
> > > > > > Try en-US before enumerating dictionaries
> > > > > > 
> > > > > > This will cause us not to pick up the dictionary after it's been installed for
> > > > > > the first time in en-US builds.
> > > > > 
> > > > > "It" being firefox?
> > > > 
> > > > No, a new dictionary.
> > > 
> > > Does installing a new dictionary (through firefox, or externally?) clear the
> > > old pref?
> > 
> > No (well, I don't think so).  If you're on a xx-YY system and an en-US firefox,
> > installing an xx-YY dictionary and restarting Firefox would lead into en-US
> > being picked up again.  The key here is dictName being empty or not.
> 
> But on exiting, Firefox would write spellchecker.dictionary="en-US", and then
> after restarting, Init() would read in spellchecker.dictionary="en-US" and skip
> the dictionary-scanning logic.  (Unless I'm missing something.)

No, it's not that simple unfortunately.  The pref is not written to if we end up picking the default locale dictionary, for example.

And here's some new bit of information: <http://mxr.mozilla.org/comm-central/search?string=UninitSpellChecker>  IOW, UninitSpellChecker is *never* called in Firefox!!!  /me wants to bang his head against the wall.
K, given all this, it definitely sounds like the closest-to-right fix for now is to have the locale accessible from content processes.  Will poke on bug 576507.
Depends on: 576507

Comment 19

9 years ago
(In reply to comment #18)
> K, given all this, it definitely sounds like the closest-to-right fix for now
> is to have the locale accessible from content processes.  Will poke on bug
> 576507.

Sounds good to me!
Fixed by bug 567507.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.