avoid dereferencing NULL extension in ExtensionHost

avoid dereferencing NULL extension in ExtensionHost

BUG=chromium-os:13739
TEST=None

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

[Denis Lagno, 2011-05-05 13:03:53]

please take a look

[Denis Lagno, 2011-05-05 13:12:52]

Aaron on extended leave.  Changing reviewer to Erik Kay.

[Erik does not do reviews, 2011-05-05 13:56:20]

+mpcomplete, akalin

In principle this seems reasonable to me, but I'd like Matt to take a closer
look first.

[Matt Perry, 2011-05-05 17:17:59]

http://codereview.chromium.org/6932038/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/6932038/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_process_manager.cc:294: if
(host->extension() == NULL ||
This seems a little dangerous to me. Here's an alternative approach:
1. remove this change.
2. change ExtensionHost to hold a scoped_refptr<const Extension> rather than a
raw ptr.
3. change ExtensionHost so that it doesn't NULL out its extension_ ptr in
EXTENSION_UNLOADED. (Extension objects were not refcounted when that was
written, so it was not possible then.)
4. have ExtensionHost set a flag when it receives EXTENSION_UNLOADED, and check
that flag in places where we currently NULL-check the extension ptr.

[akalin, 2011-05-05 18:53:17]

On 2011/05/05 17:17:59, Matt Perry wrote:
http://codereview.chromium.org/6932038/diff/2001/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_process_manager.cc:294: if
> (host->extension() == NULL ||
> This seems a little dangerous to me. Here's an alternative approach:
> 1. remove this change.
> 2. change ExtensionHost to hold a scoped_refptr<const Extension> rather than a
> raw ptr.
> 3. change ExtensionHost so that it doesn't NULL out its extension_ ptr in
> EXTENSION_UNLOADED. (Extension objects were not refcounted when that was
> written, so it was not possible then.)
> 4. have ExtensionHost set a flag when it receives EXTENSION_UNLOADED, and
check
> that flag in places where we currently NULL-check the extension ptr.

What about having ExtensionHost keep a copy of the extension ID and check that
instead?  That seems simpler to me.

[Matt Perry, 2011-05-05 19:14:14]

On 2011/05/05 18:53:17, akalin wrote:
> On 2011/05/05 17:17:59, Matt Perry wrote:
>
http://codereview.chromium.org/6932038/diff/2001/chrome/browser/extensions/ex...
> > chrome/browser/extensions/extension_process_manager.cc:294: if
> > (host->extension() == NULL ||
> > This seems a little dangerous to me. Here's an alternative approach:
> > 1. remove this change.
> > 2. change ExtensionHost to hold a scoped_refptr<const Extension> rather than
a
> > raw ptr.
> > 3. change ExtensionHost so that it doesn't NULL out its extension_ ptr in
> > EXTENSION_UNLOADED. (Extension objects were not refcounted when that was
> > written, so it was not possible then.)
> > 4. have ExtensionHost set a flag when it receives EXTENSION_UNLOADED, and
> check
> > that flag in places where we currently NULL-check the extension ptr.
> 
> What about having ExtensionHost keep a copy of the extension ID and check that
> instead?  That seems simpler to me.

Yep, that's much better!

[Denis Lagno, 2011-05-06 11:55:19]

PTAL

[Matt Perry, 2011-05-06 18:19:36]

LGTM with below fixed

http://codereview.chromium.org/6932038/diff/5003/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/6932038/diff/5003/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_host.h:75: const std::string& id() const {
I think this should be named extension_id (and the member var). ID is ambiguous
in this context.

http://codereview.chromium.org/6932038/diff/5003/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_host.h:76: DCHECK(extension_ == NULL ||
extension_->id() == id_);
nit: this is overkill IMO. I'd leave it out.

[akalin, 2011-05-06 18:40:31]

LGTM after you take care of other comments

http://codereview.chromium.org/6932038/diff/5003/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/6932038/diff/5003/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_host.h:249: std::string id_;
may as well make this const

[Denis Lagno, 2011-05-06 19:50:33]

http://codereview.chromium.org/6932038/diff/5003/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/6932038/diff/5003/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_host.h:75: const std::string& id() const {
On 2011/05/06 18:19:36, Matt Perry wrote:
> I think this should be named extension_id (and the member var). ID is
ambiguous
> in this context.

Done.

http://codereview.chromium.org/6932038/diff/5003/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_host.h:76: DCHECK(extension_ == NULL ||
extension_->id() == id_);
On 2011/05/06 18:19:36, Matt Perry wrote:
> nit: this is overkill IMO. I'd leave it out.

Done.

http://codereview.chromium.org/6932038/diff/5003/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_host.h:249: std::string id_;
On 2011/05/06 18:40:31, akalin wrote:
> may as well make this const

Done.

[Matt Perry, 2011-05-06 19:54:40]

lgtm