Remove refptr usages from SpellCheckHost

Remove refptr usages from SpellCheckHost

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

[KushalP, 2011-09-15 23:40:32]

This isn't finished yet, but posting it to be tracked.

Currently there's one issue that needs to be sorted which has appeared through
the removal of scoped_refptr.

The error:

chrome/browser/spellchecker/spellcheck_host.cc:39:10: error: no viable
conversion from
      'scoped_ptr<SpellCheckHostImpl>' to 'scoped_ptr<SpellCheckHost>'
  return host;
         ^~~~
./base/memory/scoped_ptr.h:135:3: note: candidate constructor not viable: no
known
      conversion from 'scoped_ptr<SpellCheckHostImpl>' to 'const
      scoped_ptr<SpellCheckHost> &' for 1st argument;
  scoped_ptr(const scoped_ptr&);
  ^
1 error generated.

[Hironori Bono, 2011-10-14 10:57:03]

Greetings,

Sorry for the lack of responses.
I think we also need to change the SpellChceckProfile class, which calls
SpellCheckHost::Create(), to remove scoped_refptr from this class. (I'm not sure
if ScopedRunnableMethodFactory allows posting a task to another thread, though.)

Regards,

Hironori Bono

http://codereview.chromium.org/7919003/diff/1/chrome/browser/spellchecker/spe...
File chrome/browser/spellchecker/spellcheck_host.cc (right):

http://codereview.chromium.org/7919003/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_host.cc:26: scoped_ptr<SpellCheckHost>
SpellCheckHost::Create(
When we change the return value to SpellChekHost*, this line becomes:
  SpellCheckHost* SpellCheckHost::Create(

http://codereview.chromium.org/7919003/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_host.cc:32: host.reset(
When we change the return value to SpellCheckHost*, this line becomes:
  SpellCheckHost* host =

http://codereview.chromium.org/7919003/diff/1/chrome/browser/spellchecker/spe...
File chrome/browser/spellchecker/spellcheck_host.h (right):

http://codereview.chromium.org/7919003/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_host.h:14: #include
"base/memory/ref_counted.h"
nit: we do not need this line any longer once we remove scoped_refptr<> from
this class. :)

http://codereview.chromium.org/7919003/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_host.h:64: static
scoped_ptr<SpellCheckHost> Create(
We can change this return type to SpellCheckHost* when we use scoped_ptr<>.

[dcheng, 2011-10-20 23:24:34]

Completely random drive by.

http://codereview.chromium.org/7919003/diff/7001/chrome/browser/spellchecker/...
File chrome/browser/spellchecker/spellcheck_host.cc (right):

http://codereview.chromium.org/7919003/diff/7001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_host.cc:7: #include
"base/memory/scoped_ptr.h"
I'm surprised you need to add this as we don't ever used scoped_ptr directly in
this file. Are we missing an include in another header file?

http://codereview.chromium.org/7919003/diff/7001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_host.cc:39: return (SpellCheckHost*)host;
You shouldn't need this cast.

[KushalP, 2011-10-20 23:30:38]

Updated with new patch set that compiles target 'unit_test' successfully.

It's currently running through the trybots, but I have a suspicion it'll break
as SpellCheckHostImpl crosses between FILE and UI threads.

Also adding @akalin as this is likely to require a refactoring he suggested in a
related issue, to separate thread usage with *ImplBackend and *ImplFrontend.

[KushalP, 2011-10-21 00:27:25]

I'm about to start refactoring this, but would appreciate some clarification.

SpellCheckHostImplBackend (SCIB) and SpellCheckHostImplFrontend (SCIF) will
exist on the FILE and UI threads, respectively. However which should hold a RCTS
version of the other?

SpellCheckHostImpl gets constructed on the UI thread so I was imagining it being
the main class. However, how should I mediate between the two when they're on
separate threads? What's the preferred way to structure this, and has it been
done anywhere in the codebase before?

[KushalP, 2011-10-21 00:27:26]

I'm about to start refactoring this, but would appreciate some clarification.

SpellCheckHostImplBackend (SCIB) and SpellCheckHostImplFrontend (SCIF) will
exist on the FILE and UI threads, respectively. However which should hold a RCTS
version of the other?

SpellCheckHostImpl gets constructed on the UI thread so I was imagining it being
the main class. However, how should I mediate between the two when they're on
separate threads? What's the preferred way to structure this, and has it been
done anywhere in the codebase before?

[akalin, 2011-10-21 00:31:58]

Both SCIB and SCIF are non-thread-safe on their respective threads
(i.e., not ref-counted).

The frontend has a ref-counted inner class that owns the SCIB and
constructs/destructs it on the appropriate thread.

If the frontend needs to talk to the backend, it does so by posting
tasks to its inner class.

If the backend needs to talk to the frontend, it should be either do
so via a ThreadSafeObserverList or a WeakPtr (posted on the right
thread, of course).

On Thu, Oct 20, 2011 at 5:27 PM,  <kushi.p@gmail.com> wrote:
> I'm about to start refactoring this, but would appreciate some
> clarification.
>
> SpellCheckHostImplBackend (SCIB) and SpellCheckHostImplFrontend (SCIF) will
> exist on the FILE and UI threads, respectively. However which should hold a
> RCTS
> version of the other?
>
> SpellCheckHostImpl gets constructed on the UI thread so I was imagining it
> being
> the main class. However, how should I mediate between the two when they're
> on
> separate threads? What's the preferred way to structure this, and has it
> been
> done anywhere in the codebase before?
>
> http://codereview.chromium.org/7919003/
>

[akalin, 2011-10-21 00:34:22]

If the backend stuff is mostly just one-off tasks, you may be able to
get away with PostTaskAndReply.

On Thu, Oct 20, 2011 at 5:31 PM, Fred Akalin <akalin@chromium.org> wrote:
> Both SCIB and SCIF are non-thread-safe on their respective threads
> (i.e., not ref-counted).
>
> The frontend has a ref-counted inner class that owns the SCIB and
> constructs/destructs it on the appropriate thread.
>
> If the frontend needs to talk to the backend, it does so by posting
> tasks to its inner class.
>
> If the backend needs to talk to the frontend, it should be either do
> so via a ThreadSafeObserverList or a WeakPtr (posted on the right
> thread, of course).
>
> On Thu, Oct 20, 2011 at 5:27 PM,  <kushi.p@gmail.com> wrote:
>> I'm about to start refactoring this, but would appreciate some
>> clarification.
>>
>> SpellCheckHostImplBackend (SCIB) and SpellCheckHostImplFrontend (SCIF) will
>> exist on the FILE and UI threads, respectively. However which should hold a
>> RCTS
>> version of the other?
>>
>> SpellCheckHostImpl gets constructed on the UI thread so I was imagining it
>> being
>> the main class. However, how should I mediate between the two when they're
>> on
>> separate threads? What's the preferred way to structure this, and has it
>> been
>> done anywhere in the codebase before?
>>
>> http://codereview.chromium.org/7919003/
>>
>

[akalin, 2011-10-21 00:34:56]

One example would be NonBlockingInvalidationNotifier (frontend) vs.
InvalidationNotifier (backend).

On Thu, Oct 20, 2011 at 5:34 PM, Fred Akalin <akalin@chromium.org> wrote:
> If the backend stuff is mostly just one-off tasks, you may be able to
> get away with PostTaskAndReply.
>
> On Thu, Oct 20, 2011 at 5:31 PM, Fred Akalin <akalin@chromium.org> wrote:
>> Both SCIB and SCIF are non-thread-safe on their respective threads
>> (i.e., not ref-counted).
>>
>> The frontend has a ref-counted inner class that owns the SCIB and
>> constructs/destructs it on the appropriate thread.
>>
>> If the frontend needs to talk to the backend, it does so by posting
>> tasks to its inner class.
>>
>> If the backend needs to talk to the frontend, it should be either do
>> so via a ThreadSafeObserverList or a WeakPtr (posted on the right
>> thread, of course).
>>
>> On Thu, Oct 20, 2011 at 5:27 PM,  <kushi.p@gmail.com> wrote:
>>> I'm about to start refactoring this, but would appreciate some
>>> clarification.
>>>
>>> SpellCheckHostImplBackend (SCIB) and SpellCheckHostImplFrontend (SCIF) will
>>> exist on the FILE and UI threads, respectively. However which should hold a
>>> RCTS
>>> version of the other?
>>>
>>> SpellCheckHostImpl gets constructed on the UI thread so I was imagining it
>>> being
>>> the main class. However, how should I mediate between the two when they're
>>> on
>>> separate threads? What's the preferred way to structure this, and has it
>>> been
>>> done anywhere in the codebase before?
>>>
>>> http://codereview.chromium.org/7919003/
>>>
>>
>

[Hironori Bono, 2011-10-21 08:46:36]

Greetings,

Thank you for your hard work.
This SpellCheckHostImpl class maps a dictionary file to memory when a renderer
spellchecks a word for the first time. It also downloads the dictionary file
from our server if it is not in the local storage. (Yes, this fallback logic is
complicated now.) It may be good time to simplify its logic so we can use
PostTaskAndReply()?

Regards,

Hironori Bono

[KushalP, 2011-10-21 09:09:01]

Does that mean I still have to go through the separation of SCIB and SCIF? Or is
this significantly simpler?

Looking through the header at how it works now and will shortly look through the
codebase for how it's used.

On 2011/10/21 08:46:36, hbono wrote:
> Greetings,
> 
> Thank you for your hard work.
> This SpellCheckHostImpl class maps a dictionary file to memory when a renderer
> spellchecks a word for the first time. It also downloads the dictionary file
> from our server if it is not in the local storage. (Yes, this fallback logic
is
> complicated now.) It may be good time to simplify its logic so we can use
> PostTaskAndReply()?
> 
> Regards,
> 
> Hironori Bono

[Hironori Bono, 2011-10-21 09:22:26]

Greetings,

To simply describe the intention of my previous comment, "we should redesign the
SpellCheckImpl class so it becomes simple." :)
Even though this class looks complicated, it actually consists of several
one-off tasks: mapping a file, downloading a file, opening a custom dictionary,
etc. So, to read the comment from Fred, I just thought it may be a good option
to reimplement this class with PostTaskAndReply(). (I have not tried it and it
may be wrong, though.)

Regards,

Hironori Bono

[KushalP, 2011-10-25 21:13:17]

Patch Set 5 is my first try at getting PostTaskAndReply working.

I wasn't sure how to write review comments for my own codereview, so I've just
marked them as TODO comments in the source within spellcheck_host_impl.cc.

I've sent this off for a tryrun, but I'm expecting it to fail at the
InitializeInternal() point.

Throughout the class it uses 'Initialize' in various ways:
- setting it up FROM the file thread
- downloading the words DB
- stuffing the words DB

Are there better terms for these 'actions' other than 'Initialize' ? Is there a
precedent for naming things in a domain within Chromium?

[KushalP, 2011-10-31 00:25:27]

TL;DR I think there's a mocking issue related to the IsReady() function in
spellcheck_profile_unittest.cc. How does the mocking work in this case (there's
no defined state for the bool) ?

Please see patch set 7. Specifically the spellcheck_profile_unittest.cc file for
my comments on what I've found while debugging.

Here's the output I get when I compile (and run) unit_tests:

$ ./out/Debug/unit_tests --gtest_filter=SpellCheckProfileTest.*
Note: Google Test filter = SpellCheckProfileTest.*
[==========] Running 8 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 8 tests from SpellCheckProfileTest
[ RUN      ] SpellCheckProfileTest.ReinitializeEnabled
chrome/browser/spellchecker/spellcheck_profile_unittest.cc:58: Failure
Actual function call count doesn't match EXPECT_CALL(*host, UnsetProfile())...
         Expected: to be called once
           Actual: never called - unsatisfied and active
[4630:2307:1030/195820:31921237457357:INFO:spellcheck_profile.cc(87)]
host_ready_: 0
[4630:2307:1030/195820:31921237668611:INFO:spellcheck_profile.cc(88)] host_:
0x417800
[4630:2307:1030/195820:31921237854952:INFO:spellcheck_profile.cc(89)] IsReady: 0
chrome/browser/spellchecker/spellcheck_profile_unittest.cc:119: Failure
Value of: target.IsCreatedHostReady()
  Actual: false
Expected: true
unit_tests(4630,0xa08c0540) malloc: *** error for object 0x10ec10: pointer being
freed was not allocated
*** set a breakpoint in malloc_error_break to debug
[4630:2307:1030/195820:31921273282610:ERROR:process_util_mac.mm(549)]
Terminating process due to a potential for future heap corruption
[4630:2307:1030/195820:31921281104816:ERROR:process_util_posix.cc(134)] Received
signal 10

If you look at line 108 of spellcheck_profile_unittest.cc you can see some of my
notes on why I think the test is failing. Largely, it ends up being that the
(mocked) IsReady() function doesn't show as true so it's not possible for the
target.IsCreatedHostReady() method to return true.

[KushalP, 2011-10-31 11:48:39]

target.SetHostToBeCreated(host.get()) seems to be the way to set the return
values for mocked methods.

However, given the original "pointer being freed was not allocated" error that
appeared, I think throwing this in with a loop_.RunAllPending() will not
suffice.

I'm not at home at the moment, but will have a play around later this evening.
If you guys know what's up at the moment with this, I'm all ears.

[Hironori Bono, 2011-10-31 12:30:13]

Greetings,

Thanks you so much for your updates.
I will try your change on my PC to find possible solutions for your issues
tomorrow.

Regards,

Hironori Bono

[Hironori Bono, 2011-11-01 09:58:18]

Greetings,

I have tried your change and investigated it today. It seems
SpellCheckProfileTest.* fail because of double frees. (We need to use
scoped_ptr<>::release() instead of scoped_ptr<>::get() when moving its pointer
to another scoped_ptr<>.) I have uploaded a change that applied some hacks to
avoid assertions and crashes when using the debug version of Chromium:
<http://codereview.chromium.org/8432027/>. Even though I have not tested this
change thoroughly, I wish it helps.

Regards,

Hironori Bono

[KushalP, 2011-11-01 10:56:54]

Thanks Hironori!

So it was a pointer ownership problem in the end? I thought that .reset() and
.release() were similar -- I'll have a read later today. Thanks again!

On 2011/11/01 09:58:18, hbono wrote:
> Greetings,
> 
> I have tried your change and investigated it today. It seems
> SpellCheckProfileTest.* fail because of double frees. (We need to use
> scoped_ptr<>::release() instead of scoped_ptr<>::get() when moving its pointer
> to another scoped_ptr<>.) I have uploaded a change that applied some hacks to
> avoid assertions and crashes when using the debug version of Chromium:
> <http://codereview.chromium.org/8432027/>. Even though I have not tested this
> change thoroughly, I wish it helps.
> 
> Regards,
> 
> Hironori Bono

[Hironori Bono, 2011-11-01 11:01:07]

Greetings,

It seems so. (I'm not confident of it, though.)

Regards,

Hironori Bono

On 2011/11/01 10:56:54, KushalP wrote:
> Thanks Hironori!
> 
> So it was a pointer ownership problem in the end? I thought that .reset() and
> .release() were similar -- I'll have a read later today. Thanks again!
> 
> On 2011/11/01 09:58:18, hbono wrote:
> > Greetings,
> > 
> > I have tried your change and investigated it today. It seems
> > SpellCheckProfileTest.* fail because of double frees. (We need to use
> > scoped_ptr<>::release() instead of scoped_ptr<>::get() when moving its
pointer
> > to another scoped_ptr<>.) I have uploaded a change that applied some hacks
to
> > avoid assertions and crashes when using the debug version of Chromium:
> > <http://codereview.chromium.org/8432027/>. Even though I have not tested
this
> > change thoroughly, I wish it helps.
> > 
> > Regards,
> > 
> > Hironori Bono

[KushalP, 2011-11-01 16:04:28]

Thanks (largely) to hbono,, this patch set looks good to review now!

Please see Patch Set 8 for the tryjob results. Win is orange on another
(unrelated) test.

[KushalP, 2011-11-02 23:35:14]

For the last few hours interactive_ui_tests on Win has kept the tree closed.

Hopefully when it's open we can send this patch through the bots again and those
tests will be nice and green!

[Hironori Bono, 2011-11-04 03:46:59]

Greetings,

Thank you so much for your update. Even though the latest one looks good to me,
it causes a conflict on the latest repository. Is it possible to re-sync your
repository, fix the conflict, and re-upload the change?

Regards,

Hironori Bono

http://codereview.chromium.org/7919003/diff/33001/chrome/browser/spellchecker...
File chrome/browser/spellchecker/spellcheck_host.h (left):

http://codereview.chromium.org/7919003/diff/33001/chrome/browser/spellchecker...
chrome/browser/spellchecker/spellcheck_host.h:53:
BrowserThread::DeleteOnFileThread> {
nit: 'BrowserThread' has been renamed to 'content::BrowserThread'. Is it
possible to re-sync your repository to the latest one and re-upload this change?

http://codereview.chromium.org/7919003/diff/33001/chrome/browser/spellchecker...
File chrome/browser/spellchecker/spellcheck_host_impl.cc (right):

http://codereview.chromium.org/7919003/diff/33001/chrome/browser/spellchecker...
chrome/browser/spellchecker/spellcheck_host_impl.cc:421: 
nit: sorry, I forgot answering your question why we call Initialize() here
again. As you know, this function is called when we finish loading a dictionary
from the server and save it. For simplicity, we call Initialize() to
re-initialize the SpellCheckHostImpl object with the loaded dictionary.

[KushalP, 2011-11-04 23:53:19]

@Hironori Thanks for the explanation of Initialize! I still think it should be
named something more obvious/specific.

Rebased to Trunk and ran against trybots. This looks a lot better.

[Hironori Bono, 2011-11-07 04:50:11]

LGTM.
Thank you for your great work. Can you land this change by yourself? Or shall I
land it on your behalf? 

Regards,

Hironori Bono

[Hironori Bono, 2011-11-07 07:59:59]

Greetings,

Sorry, I notice this code causes a DCHECK failure after downloading a
dictionary. To fix this DCHECK failure, we need to add a function
SpellCheckHostImpl::SaveDictionaryDataComplete and a boolean variable
|dictionary_saved_| as written in my comments. Is it possible to fix it?

Regards,

Hironori Bono

http://codereview.chromium.org/7919003/diff/40001/chrome/browser/spellchecker...
File chrome/browser/spellchecker/spellcheck_host_impl.cc (right):

http://codereview.chromium.org/7919003/diff/40001/chrome/browser/spellchecker...
chrome/browser/spellchecker/spellcheck_host_impl.cc:104:
weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
Can you initialize |dictionary_saved_| to false here?

http://codereview.chromium.org/7919003/diff/40001/chrome/browser/spellchecker...
chrome/browser/spellchecker/spellcheck_host_impl.cc:368: data_ = data;
Is it possible to add a line 'dictionary_saved_ = false;' here?

http://codereview.chromium.org/7919003/diff/40001/chrome/browser/spellchecker...
chrome/browser/spellchecker/spellcheck_host_impl.cc:372:
base::Bind(&SpellCheckHostImpl::InformProfileOfInitialization,
Can you add a new function 'SaveDictionaryDataComplete' to this class and use it
instead of InformProfileOfInitialization?

http://codereview.chromium.org/7919003/diff/40001/chrome/browser/spellchecker...
chrome/browser/spellchecker/spellcheck_host_impl.cc:424: Initialize();
Sorry, I have noticed this Initialize() call causes a DCHECK failure. Is it
possible to add a boolean variable |dictionary_saved_| and set it true here?

http://codereview.chromium.org/7919003/diff/40001/chrome/browser/spellchecker...
chrome/browser/spellchecker/spellcheck_host_impl.cc:469: }
Can you add the implementation of the SaveDictionaryDataComplete() here as
listed in the following snippet?

  void SpellCheckHostImpl::SaveDictionaryDataComplete() {
    if (dictionary_saved_)
      Initialize();
    else
      InformProfileOfInitialization();
  }

[KushalP, 2011-11-07 11:25:29]

Sure, I'll get onto this later tonight.

Just to be clear, is the dictionary_saved_ boolean going to be used as a DCHECK
at the start of the Initialize() function?

[KushalP, 2011-11-08 00:14:21]

I've added the SaveDictionaryDataComplete function and the patch is currently
running through the trybots.

It looks as though this will clear them fine.

[Hironori Bono, 2011-11-08 07:20:25]

LGTM again with a couple of nits.
Thank you so much for your great work. It is awesome.

Regards,

Hironori Bono

http://codereview.chromium.org/7919003/diff/48001/chrome/browser/spellchecker...
File chrome/browser/spellchecker/spellcheck_host_impl.h (right):

http://codereview.chromium.org/7919003/diff/48001/chrome/browser/spellchecker...
chrome/browser/spellchecker/spellcheck_host_impl.h:79: void
AddWordComplete(const std::string& word);
nit: can you add a comment for this function, such as 'Called when we finish
adding a word in the background'?

http://codereview.chromium.org/7919003/diff/48001/chrome/browser/spellchecker...
chrome/browser/spellchecker/spellcheck_host_impl.h:134: bool dictionary_saved_;
nit: can you add a comment for this variable, such as 'Whether we successfully
download a dictionary and save it. This variable is used in a callback function
SaveDictionaryDataComplete()'?

[commit-bot: I haz the power, 2011-11-08 10:16:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kushi.p@gmail.com/7919003/52001

[commit-bot: I haz the power, 2011-11-08 12:02:25]

Try job failure for 7919003-52001 (retry) on win_rel for step
"chrome_frame_net_tests".
It's a second try, previously, step "chrome_frame_net_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-11-08 12:34:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kushi.p@gmail.com/7919003/52001

[commit-bot: I haz the power, 2011-11-08 14:15:45]

Change committed as 109022