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

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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?

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oops, meant to CC mfinkle but mid-aired with ehsan.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Er, 

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

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Try en-US before enumerating 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.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Don't save the dictionary to a pref if it didn't change

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.

Comment 7

[(no longer active)]

(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

[(no longer active)]

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.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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

[(no longer active)]

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?

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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

[(no longer active)]

(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.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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

[(no longer active)]

(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

[(no longer active)]

(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.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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

[(no longer active)]

(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.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 19

[(no longer active)]

(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!

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Fixed by bug 567507.