[chromium-reviews] Fix a spell check dictionary download bug, where killing the spell checker wh...

Fix a spell check dictionary download bug, where killing the spell checker while downloading the dictionary leads to a crash. This is happening for my auto-language detection feature, which sometimes kills a spellchecker while it is downloading a dictionary file. Hopefully, this is also a fix for BUG 18743.

BUG=www.crbug.com/18743
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=23048

[sidchat (Google), 2009-08-07 23:04:28]

Peter suggested that hunspell initialization can be moved to file thread after
callback from url fetcher in the IO thread, so that all jankiness is removed. I
will experiment with that in a separate CL.

[Peter Kasting, 2009-08-07 23:14:24]

This change would be fine except it needs to untangle what I think was too
tangled even before.

http://codereview.chromium.org/165175/diff/1/2
File chrome/browser/spellchecker.cc (right):

http://codereview.chromium.org/165175/diff/1/2#newcode473
Line 473: Task* dic_task =
dic_download_state_changer_factory_.NewRunnableMethod(
All this stuff seems totally unnecessary.  You can remove all these factories
and proxies and objects and just deal with everything in this thread.

Looking at some of the code for this DictionaryDownloader in spellchecker.cc
this is a rat's nest.  Simplify.

http://codereview.chromium.org/165175/diff/1/2#newcode478
Line 478: MessageLoop::current()->PostTask(FROM_HERE,
NewRunnableMethod(ddc_dic_.get(),
You don't need to post a task for this.  If it just calls Start() on a URL
fetcher it won't take any time, and you'll save one message loop item.

[sidchat (Google), 2009-08-08 00:00:34]

OK, completely destroyed the rat's nest - tested by randomly changing spellcheck
language and typing - no jankiness - all other functionalities remain intact.

Please have a look again.

-Sid

On 2009/08/07 23:14:24, Peter Kasting wrote:
> This change would be fine except it needs to untangle what I think was too
> tangled even before.
> 
> http://codereview.chromium.org/165175/diff/1/2
> File chrome/browser/spellchecker.cc (right):
> 
> http://codereview.chromium.org/165175/diff/1/2#newcode473
> Line 473: Task* dic_task =
> dic_download_state_changer_factory_.NewRunnableMethod(
> All this stuff seems totally unnecessary.  You can remove all these factories
> and proxies and objects and just deal with everything in this thread.
> 
> Looking at some of the code for this DictionaryDownloader in spellchecker.cc
> this is a rat's nest.  Simplify.
> 
> http://codereview.chromium.org/165175/diff/1/2#newcode478
> Line 478: MessageLoop::current()->PostTask(FROM_HERE,
> NewRunnableMethod(ddc_dic_.get(),
> You don't need to post a task for this.  If it just calls Start() on a URL
> fetcher it won't take any time, and you'll save one message loop item.

[Peter Kasting, 2009-08-08 00:20:03]

Dramatically better.  Comments below.

http://codereview.chromium.org/165175/diff/1008/8
File chrome/browser/spellchecker.cc (right):

http://codereview.chromium.org/165175/diff/1008/8#newcode92
Line 92: return file_util::WriteFile(file_to_write, data.data(), num_bytes) ==
It worries me to see us writing things to disk on the IO thread.  Disk
operations can take arbitrary amounts of time, and we can't afford that.  At
least this part seems like maybe it should be done on the File thread.  Brett
should comment.  I suppose you could do this in a later pass, but I'd like to
see it happen imminently... probably the same applies to the Hunspell
initialization, which at a glance looks like it reads from disk.

http://codereview.chromium.org/165175/diff/1008/9
File chrome/browser/spellchecker.h (right):

http://codereview.chromium.org/165175/diff/1008/9#newcode139
Line 139: void set_file_is_downloading(bool value);
You can remove this function in favor of just setting the variable directly, now
that it's all done by code internal to this class.

[sidchat (Google), 2009-08-10 18:34:59]

Right - so I removed the set_file_is_downloading() function. I set a TODO for
save-to-disk in the file thread - will do it in another CL asap after this one.
Please ignore the try bot update error - its not mine it seems :)

Please take another look.

-Sid

On 2009/08/08 00:20:03, Peter Kasting wrote:
> Dramatically better.  Comments below.
> 
> http://codereview.chromium.org/165175/diff/1008/8
> File chrome/browser/spellchecker.cc (right):
> 
> http://codereview.chromium.org/165175/diff/1008/8#newcode92
> Line 92: return file_util::WriteFile(file_to_write, data.data(), num_bytes) ==
> It worries me to see us writing things to disk on the IO thread.  Disk
> operations can take arbitrary amounts of time, and we can't afford that.  At
> least this part seems like maybe it should be done on the File thread.  Brett
> should comment.  I suppose you could do this in a later pass, but I'd like to
> see it happen imminently... probably the same applies to the Hunspell
> initialization, which at a glance looks like it reads from disk.
> 
> http://codereview.chromium.org/165175/diff/1008/9
> File chrome/browser/spellchecker.h (right):
> 
> http://codereview.chromium.org/165175/diff/1008/9#newcode139
> Line 139: void set_file_is_downloading(bool value);
> You can remove this function in favor of just setting the variable directly,
now
> that it's all done by code internal to this class.

[Peter Kasting, 2009-08-10 22:25:43]

http://codereview.chromium.org/165175/diff/1011/1013
File chrome/browser/spellchecker.h (right):

http://codereview.chromium.org/165175/diff/1011/1013#newcode115
Line 115: // URL Fetcher delegate implementation.
Nit: I suggest changing this to:

// URLFetcher::Delegate implementation.  Called when we finish downloading a
spellcheck dictionary; saves the dictionary to disk.
// TODO(sidchat): Save to disk in the file thread instead of the IO thread.

http://codereview.chromium.org/165175/diff/1011/1013#newcode155
Line 155: // Start the dictionary download process in the file thread. The
UrlFetcher
Nit: This comment is no longer true because the download process is not started
in the file thread.  Also, to be technically correct, the URLFetcher does not
live in the IO thread; the URLFetcher::Core does.  I suggest the following
comment, assuming you follow my OnURLFetchComplete() comment suggestion above:

// Start downloading a dictionary from the server.  On completion, the
OnURLFetchComplete() function is invoked.

[Peter Kasting, 2009-08-10 22:25:59]

BTW, LGTM, forgot that bit.