LocationBarView::PageActionImageView::LoadImageTask::Run() may pass a NULL im...

LocationBarView::PageActionImageView::LoadImageTask::Run() may pass a NULL image pointer through to OnImageLoaded; make sure that the pointer is not dereferenced in these cases.

BUG= http://crbug.com/18140
TEST= none.


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

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

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

[Miranda Callahan, 2009-07-31 21:42:18]

I wish I could repro this bug.  It's pretty clear that the problem is a null
image pointer, but I couldn't sneak one in to test.  

I decided to fix by inserting an empty SkBitmap instead of stopping everything
in its tracks because this is a little bit more forgiving; if the problem is
just a one-time issue, we can silently overlook the faux pas, and wait for
success the next time the location_bar_view code tries to load the image; if
there really is a problem with the file, on Chrome restart the problem extension
will refuse to load and inform the user.

Please look carefully at this change -- even though it's one line, I couldn't
test it, and I am not deeply familiar with the extension system.

(Also, Glen, I know you are busy -- feel free to suggest an alternate
reviewer...)

[Glen Murphy, 2009-07-31 22:36:06]

> Please look carefully at this change -- even though it's 
> one line, I couldn't test it, and I am not deeply familiar
> with the extension system.

Assuming it was tested by taking out the if to sees what happens if the clause
is hit, LGTM.

Finnur, despite my LGTM, please take a look at this when you get back; this code
melts my attention-deprived brain.



On 2009/07/31 21:42:18, Miranda Callahan wrote:
> I wish I could repro this bug.  It's pretty clear that the problem is a null
> image pointer, but I couldn't sneak one in to test.  
> 
> I decided to fix by inserting an empty SkBitmap instead of stopping everything
> in its tracks because this is a little bit more forgiving; if the problem is
> just a one-time issue, we can silently overlook the faux pas, and wait for
> success the next time the location_bar_view code tries to load the image; if
> there really is a problem with the file, on Chrome restart the problem
extension
> will refuse to load and inform the user.
> 
> Please look carefully at this change -- even though it's one line, I couldn't
> test it, and I am not deeply familiar with the extension system.
> 
> (Also, Glen, I know you are busy -- feel free to suggest an alternate
> reviewer...)

[Miranda Callahan, 2009-07-31 22:53:28]

> Assuming it was tested by taking out the if to sees what happens if the clause
> is hit, LGTM.

Yep, I tested this clause and it seems to behave itself with an empty SkBitmap. 
Thanks, Glen -- I will also wait for Finnur to take a look before committing.

[Glen Murphy, 2009-07-31 22:55:28]

Finnur's not here, and I think Anthony wanted this change pretty
quickly. Let finnur review it post-commit.

On Fri, Jul 31, 2009 at 3:53 PM, <mirandac@chromium.org> wrote:
>> Assuming it was tested by taking out the if to sees what happens if
>
> the clause
>>
>> is hit, LGTM.
>
> Yep, I tested this clause and it seems to behave itself with an empty
> SkBitmap. =A0Thanks, Glen -- I will also wait for Finnur to take a look
> before committing.
>
> http://codereview.chromium.org/159711
>

[Miranda Callahan, 2009-07-31 22:57:45]

Sure thing -- committing underway.

On 2009/07/31 22:55:28, Glen Murphy wrote:
> Finnur's not here, and I think Anthony wanted this change pretty
> quickly. Let finnur review it post-commit.
> 
> On Fri, Jul 31, 2009 at 3:53 PM, <mirandac@chromium.org> wrote:
> >> Assuming it was tested by taking out the if to sees what happens if
> >
> > the clause
> >>
> >> is hit, LGTM.
> >
> > Yep, I tested this clause and it seems to behave itself with an empty
> > SkBitmap. =A0Thanks, Glen -- I will also wait for Finnur to take a look
> > before committing.
> >
> > http://codereview.chromium.org/159711
> >

[Finnur, 2009-08-01 00:48:47]

LGTM, one nit.

http://codereview.chromium.org/159711/diff/5/6
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/159711/diff/5/6#newcode1156
Line 1156: void OnImageLoaded(SkBitmap* image, int index) {
This is fine; thanks Miranda. I think it should be easy to repro this by
specifying an image that fails to decode or maybe even one that doesn't exist
(?).

I'd like to see a NOTREACHED inside the if you just added, though.

[Miranda Callahan, 2009-08-02 19:01:11]

Hi, Finnur --

added the NOTREACHED.  I had been trying to repro with a nonexistent image,
which didn't work -- using an image that fails to decode did the trick.  This
fix indeed seems to stop the crasher.  Thanks!

On 2009/08/01 00:48:47, Finnur wrote:
> LGTM, one nit.
> 
> http://codereview.chromium.org/159711/diff/5/6
> File chrome/browser/views/location_bar_view.cc (right):
> 
> http://codereview.chromium.org/159711/diff/5/6#newcode1156
> Line 1156: void OnImageLoaded(SkBitmap* image, int index) {
> This is fine; thanks Miranda. I think it should be easy to repro this by
> specifying an image that fails to decode or maybe even one that doesn't exist
> (?).
> 
> I'd like to see a NOTREACHED inside the if you just added, though.

[Finnur, 2009-08-02 21:11:03]

Glad to hear it (I thought I had tested non-existing files, but apparently I
failed to test images that fail to decode) :)

Anyway -- The NOTREACHED helps, but I think we should be more descriptive when
it hits. I think the syntax is something like this:

NOTREACHED() << "Image failed to decode";

(Don't quote me on that though) :)


On 2009/08/02 19:01:11, Miranda Callahan wrote:
> Hi, Finnur --
> 
> added the NOTREACHED.  I had been trying to repro with a nonexistent image,
> which didn't work -- using an image that fails to decode did the trick.  This
> fix indeed seems to stop the crasher.  Thanks!
> 
> On 2009/08/01 00:48:47, Finnur wrote:
> > LGTM, one nit.
> > 
> > http://codereview.chromium.org/159711/diff/5/6
> > File chrome/browser/views/location_bar_view.cc (right):
> > 
> > http://codereview.chromium.org/159711/diff/5/6#newcode1156
> > Line 1156: void OnImageLoaded(SkBitmap* image, int index) {
> > This is fine; thanks Miranda. I think it should be easy to repro this by
> > specifying an image that fails to decode or maybe even one that doesn't
exist
> > (?).
> > 
> > I'd like to see a NOTREACHED inside the if you just added, though.

[Miranda Callahan, 2009-08-03 15:34:41]

That makes sense -- done and committed.

On 2009/08/02 21:11:03, Finnur wrote:
> Glad to hear it (I thought I had tested non-existing files, but apparently I
> failed to test images that fail to decode) :)
> 
> Anyway -- The NOTREACHED helps, but I think we should be more descriptive when
> it hits. I think the syntax is something like this:
> 
> NOTREACHED() << "Image failed to decode";
> 
> (Don't quote me on that though) :)
> 
> 
> On 2009/08/02 19:01:11, Miranda Callahan wrote:
> > Hi, Finnur --
> > 
> > added the NOTREACHED.  I had been trying to repro with a nonexistent image,
> > which didn't work -- using an image that fails to decode did the trick. 
This
> > fix indeed seems to stop the crasher.  Thanks!
> > 
> > On 2009/08/01 00:48:47, Finnur wrote:
> > > LGTM, one nit.
> > > 
> > > http://codereview.chromium.org/159711/diff/5/6
> > > File chrome/browser/views/location_bar_view.cc (right):
> > > 
> > > http://codereview.chromium.org/159711/diff/5/6#newcode1156
> > > Line 1156: void OnImageLoaded(SkBitmap* image, int index) {
> > > This is fine; thanks Miranda. I think it should be easy to repro this by
> > > specifying an image that fails to decode or maybe even one that doesn't
> exist
> > > (?).
> > > 
> > > I'd like to see a NOTREACHED inside the if you just added, though.