app_list: OnExtensionLoaded calls UpdateResults asynchronously rather than directly.

app_list: OnExtensionLoaded calls UpdateResults asynchronously rather than directly.

This fixes a crash where clicking on a search result causes an extension to be
re-enabled, and UpdateResults clears the results while one is being opened. This
change delays the call to UpdateResults until after the app has been opened.

BUG=442453
Committed: https://crrev.com/37eb085caf32580522238d12f9d906401f1f00c9
Cr-Commit-Position: refs/heads/master@{#310236}

[xiyuan, 2015-01-06 18:22:40]

xiyuan@chromium.org changed reviewers:
+ mgiuca@chromium.org

[Matt Giuca, 2015-01-07 03:03:59]

mgiuca@chromium.org changed reviewers:
+ tapted@chromium.org

[Matt Giuca, 2015-01-07 03:04:00]

Seems good. Thanks for tracking this down and fixing it!

Just some issue with the CL description. "Decouple" has a specific engineering
meaning (to remove implicit relationships between two pieces of code) and that
*isn't* what you're doing here. Please rephrase to describe what is really going
on. Suggestion:

"OnExtensionLoaded calls UpdateResults asynchronously rather than directly.

This fixes a crash where clicking on a search result causes an extension to be
re-enabled, and UpdateResults clears the results while one is being opened. This
change delays the call to UpdateResults until after the app has been opened."

lgtm, but adding tapted to have a think about this and confirm.

[xiyuan, 2015-01-07 03:29:51]

CL description updated and fixed a unit test (only tested with AppList* and the
broken one is AppSearch*). :)

I will wait for tapted's thoughts before landing this.

[tapted, 2015-01-07 04:12:31]

looks good - just one obscure corner case - which might not actually be a thing
(my WeakPtr fu is uhhh "weak").

https://codereview.chromium.org/800023004/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/app_search_provider.cc (right):

https://codereview.chromium.org/800023004/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_search_provider.cc:108:
update_results_factory_.InvalidateWeakPtrs();
TBH I haven't used WeakPtrs in Chrome all that much. But I think this step is
here to handle an UpdateResults call that doesn't come asynchronously via the
closure (otherwise it would only invalidate the weak pointer in the closure,
which is about to be cleaned up anyway).

However, in that case, it would be possible for things to be ordered like

OnExtensionLoaded [queue UpdateResults]
explicit UpdateResults (via Start()?) [invalidate queue]
OnExtensionLoaded [skip queuing UpdateResult since HasWeakPtrs]
async UpdateResults also skipped because it got invalidated
* still missing UpdateResults to handle the second OnExtensionLoaded


I *think* this line can just be removed to fix that case. The downside could be
that we do two UpdateResults() in a row without a refresh, but that's fine.

[xiyuan, 2015-01-07 04:51:57]

Thanks for chiming in. :)

https://codereview.chromium.org/800023004/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/app_search_provider.cc (right):

https://codereview.chromium.org/800023004/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_search_provider.cc:108:
update_results_factory_.InvalidateWeakPtrs();
On 2015/01/07 04:12:30, tapted wrote:
> TBH I haven't used WeakPtrs in Chrome all that much. But I think this step is
> here to handle an UpdateResults call that doesn't come asynchronously via the
> closure (otherwise it would only invalidate the weak pointer in the closure,
> which is about to be cleaned up anyway).
> 
> However, in that case, it would be possible for things to be ordered like
> 
> OnExtensionLoaded [queue UpdateResults]
> explicit UpdateResults (via Start()?) [invalidate queue]
> OnExtensionLoaded [skip queuing UpdateResult since HasWeakPtrs]
> async UpdateResults also skipped because it got invalidated
> * still missing UpdateResults to handle the second OnExtensionLoaded
> 
> 
> I *think* this line can just be removed to fix that case. The downside could
be
> that we do two UpdateResults() in a row without a refresh, but that's fine.

WeakPtrFactory::InvalidateWeakPtrs would mark the WeakPtr flag invalid and
release its reference to the flag. This way, all previous WeakPtrs returned from
its GetWeakPtr() call become invalid and unusable. And HasWeakPtrs would return
false because the flag is gone and the factory does not hold any WeakPtr.

So the case you worry about actually works. The explicit UpdateResults call
invalidates previous WeakPtr and effectively cancels the previous queued
UpdateResults call. The next OnExtensionLoaded would queue a new call because
HasWeakPtrs returns false now.

[tapted, 2015-01-07 05:03:34]

sweeet! lgtm

On 2015/01/07 04:51:57, xiyuan wrote:
> Thanks for chiming in. :)
> 
>
https://codereview.chromium.org/800023004/diff/20001/chrome/browser/ui/app_li...
> File chrome/browser/ui/app_list/search/app_search_provider.cc (right):
> 
>
https://codereview.chromium.org/800023004/diff/20001/chrome/browser/ui/app_li...
> chrome/browser/ui/app_list/search/app_search_provider.cc:108:
> update_results_factory_.InvalidateWeakPtrs();
> On 2015/01/07 04:12:30, tapted wrote:
> > TBH I haven't used WeakPtrs in Chrome all that much. But I think this step
is
> > here to handle an UpdateResults call that doesn't come asynchronously via
the
> > closure (otherwise it would only invalidate the weak pointer in the closure,
> > which is about to be cleaned up anyway).
> > 
> > However, in that case, it would be possible for things to be ordered like
> > 
> > OnExtensionLoaded [queue UpdateResults]
> > explicit UpdateResults (via Start()?) [invalidate queue]
> > OnExtensionLoaded [skip queuing UpdateResult since HasWeakPtrs]
> > async UpdateResults also skipped because it got invalidated
> > * still missing UpdateResults to handle the second OnExtensionLoaded
> > 
> > 
> > I *think* this line can just be removed to fix that case. The downside could
> be
> > that we do two UpdateResults() in a row without a refresh, but that's fine.
> 
> WeakPtrFactory::InvalidateWeakPtrs would mark the WeakPtr flag invalid and
> release its reference to the flag. 

Ahhhhh - I saw WeakReference::Flag::Invalidate() but missed
WeakReference::Invalidate() - so thought it only did the first bit

> This way, all previous WeakPtrs returned from
> its GetWeakPtr() call become invalid and unusable. And HasWeakPtrs would
return
> false because the flag is gone and the factory does not hold any WeakPtr.
> 
> So the case you worry about actually works. The explicit UpdateResults call
> invalidates previous WeakPtr and effectively cancels the previous queued
> UpdateResults call. The next OnExtensionLoaded would queue a new call because
> HasWeakPtrs returns false now.

[Matt Giuca, 2015-01-07 05:09:05]

Cool, looks good!

[xiyuan, 2015-01-07 05:25:03]

Thanks, guys.

[xiyuan, 2015-01-07 05:25:30]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2015-01-07 05:26:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/800023004/20001

[commit-bot: I haz the power, 2015-01-07 05:36:38]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-07 05:37:39]

Patchset 2 (id:??) landed as
https://crrev.com/37eb085caf32580522238d12f9d906401f1f00c9
Cr-Commit-Position: refs/heads/master@{#310236}