Don't report origin changes during loading.

Don't report origin changes during loading.

BUG=365762, 374995
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272387

[erikwright (departed), 2014-05-21 14:32:22]

pkasting: PTAL.

[Peter Kasting, 2014-05-21 19:38:41]

https://codereview.chromium.org/296053007/diff/20001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/296053007/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:1680: &dsp_change_origin_,
DSP_CHANGE_INITIAL_LOAD);
Can't we just move the |loaded_| assignment statement below this block, and then
check the |loaded_| variable below instead of adding this new enum value?

If I'm not mistaken, you don't ever want to record a change that happens
pre-database load?

https://codereview.chromium.org/296053007/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:1969: void*
previous_default_search_engine = default_search_provider_;
If you're only doing a pointer comparison, you should be able to leave this as a
TemplateURL*.  Freeing the memory the pointer points to will not invalidate the
pointer, so there's no UAF danger.

https://codereview.chromium.org/296053007/diff/20001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/296053007/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.h:465:
DSP_CHANGE_NEW_ENGINE_NO_PREFS,
This value no longer seems to be in use.  Maybe it should be removed.

[erikwright (departed), 2014-05-21 20:38:48]

https://codereview.chromium.org/296053007/diff/20001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/296053007/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:1680: &dsp_change_origin_,
DSP_CHANGE_INITIAL_LOAD);
On 2014/05/21 19:38:41, Peter Kasting wrote:
> Can't we just move the |loaded_| assignment statement below this block, and
then
> check the |loaded_| variable below instead of adding this new enum value?
> 
> If I'm not mistaken, you don't ever want to record a change that happens
> pre-database load?

ApplyDefaultSearchChange behaves differently before/after loading completes. A
bunch of other code does, too. It's definitely not as easy as moving the loaded
statement - it would have at least some ripple effects.

https://codereview.chromium.org/296053007/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:1969: void*
previous_default_search_engine = default_search_provider_;
On 2014/05/21 19:38:41, Peter Kasting wrote:
> If you're only doing a pointer comparison, you should be able to leave this as
a
> TemplateURL*.  Freeing the memory the pointer points to will not invalidate
the
> pointer, so there's no UAF danger.

This pointer could be invalidated in a variety of ways between here and when it
is used for comparison, and this method has no easy way to detect when that has
happened and set it to NULL.

It seems quite plausible to me that someone could some day introduce a check
using the pre-pop ID, the sync GUID, the id(), or other properties, which would
be a potential UAF.

By storing it as a void* we eliminate the risk of accidentally dereferencing it
while preserving the ability to do the comparison.

https://codereview.chromium.org/296053007/diff/20001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/296053007/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.h:465:
DSP_CHANGE_NEW_ENGINE_NO_PREFS,
On 2014/05/21 19:38:41, Peter Kasting wrote:
> This value no longer seems to be in use.  Maybe it should be removed.

Several of the values are unused now. I can remove them (in fact, I have
prepared a CL to mark them as deprecated in histograms.xml) but I would need to
insert placeholders to keep UMA data consistent.

[Peter Kasting, 2014-05-21 20:45:13]

https://codereview.chromium.org/296053007/diff/20001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/296053007/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:1680: &dsp_change_origin_,
DSP_CHANGE_INITIAL_LOAD);
On 2014/05/21 20:38:48, erikwright wrote:
> On 2014/05/21 19:38:41, Peter Kasting wrote:
> > Can't we just move the |loaded_| assignment statement below this block, and
> then
> > check the |loaded_| variable below instead of adding this new enum value?
> > 
> > If I'm not mistaken, you don't ever want to record a change that happens
> > pre-database load?
> 
> ApplyDefaultSearchChange behaves differently before/after loading completes. A
> bunch of other code does, too. It's definitely not as easy as moving the
loaded
> statement - it would have at least some ripple effects.

My primary concern is really that you're adding a value to an enum that's kept
in sync with histograms.xml -- but this isn't actually a histogrammed value. 
This makes the two diverge some.  For that reason alone, I'd probably do this as
a separate bool instead.

One reason I asked about values in the enum no longer being in use is because in
theory you could stuff this into one of those slots, if you need a placeholder
anyway; but I worry that might make things _really_ confusing to a reader.

https://codereview.chromium.org/296053007/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:1969: void*
previous_default_search_engine = default_search_provider_;
On 2014/05/21 20:38:48, erikwright wrote:
> This pointer could be invalidated in a variety of ways between here and when
it
> is used for comparison, and this method has no easy way to detect when that
has
> happened and set it to NULL.

Right, I understand that.

> It seems quite plausible to me that someone could some day introduce a check
> using the pre-pop ID, the sync GUID, the id(), or other properties, which
would
> be a potential UAF.
> 
> By storing it as a void* we eliminate the risk of accidentally dereferencing
it
> while preserving the ability to do the comparison.

I just think it would be better to store it as the correct type and comment that
it needs to be used only for a pointer comparison below and not dereferenced. 
To me, that's clearer.  Maybe I just have a nails-on-chalkboard reaction to any
use of void*.

Even if you used void*, the comment above is not clear; it makes it sound as if
not using void* will _cause_ a UAF, not just allow someone else to theoretically
add one.  You need a comment that's a little more fleshed-out.  And by the time
you have that, you have what you need to just make this non-void*, IMO.

Whatever.

https://codereview.chromium.org/296053007/diff/20001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/296053007/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.h:465:
DSP_CHANGE_NEW_ENGINE_NO_PREFS,
On 2014/05/21 20:38:48, erikwright wrote:
> On 2014/05/21 19:38:41, Peter Kasting wrote:
> > This value no longer seems to be in use.  Maybe it should be removed.
> 
> Several of the values are unused now. I can remove them (in fact, I have
> prepared a CL to mark them as deprecated in histograms.xml) but I would need
to
> insert placeholders to keep UMA data consistent.

That still seems preferable.  OK to do it separately if you want.

[erikwright (departed), 2014-05-21 21:18:00]

https://codereview.chromium.org/296053007/diff/20001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/296053007/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:1680: &dsp_change_origin_,
DSP_CHANGE_INITIAL_LOAD);
On 2014/05/21 20:45:13, Peter Kasting wrote:
> On 2014/05/21 20:38:48, erikwright wrote:
> > On 2014/05/21 19:38:41, Peter Kasting wrote:
> > > Can't we just move the |loaded_| assignment statement below this block,
and
> > then
> > > check the |loaded_| variable below instead of adding this new enum value?
> > > 
> > > If I'm not mistaken, you don't ever want to record a change that happens
> > > pre-database load?
> > 
> > ApplyDefaultSearchChange behaves differently before/after loading completes.
A
> > bunch of other code does, too. It's definitely not as easy as moving the
> loaded
> > statement - it would have at least some ripple effects.
> 
> My primary concern is really that you're adding a value to an enum that's kept
> in sync with histograms.xml -- but this isn't actually a histogrammed value. 
> This makes the two diverge some.  For that reason alone, I'd probably do this
as
> a separate bool instead.
> 
> One reason I asked about values in the enum no longer being in use is because
in
> theory you could stuff this into one of those slots, if you need a placeholder
> anyway; but I worry that might make things _really_ confusing to a reader.

I agree with deprecating the old values (in a separate CL).

I don't think re-using a slot makes anything simpler.

Changing the semantics of the "loaded_" bit scares me.

How about inserting a "don't send" value in the enum, but as "-1". So it will be
in the very beginning, separate from the others, and clearly different by dint
of being negative.

[Peter Kasting, 2014-05-21 21:21:05]

On 2014/05/21 21:18:00, erikwright wrote:
> How about inserting a "don't send" value in the enum, but as "-1". So it will
be
> in the very beginning, separate from the others, and clearly different by dint
> of being negative.

Why not just pass a bool arg to ApplyDefaultSearchChange (allow_logging, or
whatever)?  This seems better than keeping state in the members of the class.

[erikwright (departed), 2014-05-21 21:23:02]

On 2014/05/21 21:21:05, Peter Kasting wrote:
> On 2014/05/21 21:18:00, erikwright wrote:
> > How about inserting a "don't send" value in the enum, but as "-1". So it
will
> be
> > in the very beginning, separate from the others, and clearly different by
dint
> > of being negative.
> 
> Why not just pass a bool arg to ApplyDefaultSearchChange (allow_logging, or
> whatever)?  This seems better than keeping state in the members of the class.

Touches a bunch of call-sites for no reason. But sure I'll update it that way.

[erikwright (departed), 2014-05-22 17:45:43]

pkasting: PTAL.

[Peter Kasting, 2014-05-22 17:48:38]

LGTM

[erikwright (departed), 2014-05-22 17:50:34]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-05-22 17:53:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikwright@chromium.org/296053007/80001

[commit-bot: I haz the power, 2014-05-23 01:03:00]

Change committed as 272387