Check for NULL extension_service and extension_process_map for Android platform

Check for NULL extension_service and extension_process_map for Android platform

BUG=153582
TEST=None


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

[whywhat, 2012-10-03 19:36:49]

Please, take a look.
I'm not sure what's the best way to test this so it doesn't happen again (as I
see from commit logs, Chromium changes that assume extension_service can't null
breaking Android builds happen quite often).

[jam, 2012-10-03 21:00:13]

On 2012/10/03 19:36:49, whywhat wrote:
> Please, take a look.
> I'm not sure what's the best way to test this so it doesn't happen again (as I
> see from commit logs, Chromium changes that assume extension_service can't
null
> breaking Android builds happen quite often).

best way is to have tests that exercise that path on the chromium tree, so that
if someone does this the android try bot turns red..

https://codereview.chromium.org/11013031/diff/1/chrome/browser/memory_details.cc
File chrome/browser/memory_details.cc (right):

https://codereview.chromium.org/11013031/diff/1/chrome/browser/memory_details...
chrome/browser/memory_details.cc:232: if (extension_service != NULL)
nit: chrome style is to skip the "!= NULL"

https://codereview.chromium.org/11013031/diff/1/chrome/browser/memory_details...
chrome/browser/memory_details.cc:255: } else if (extension_process_map != NULL
&&
ditto (also below)

[whywhat, 2012-10-04 09:58:31]

https://codereview.chromium.org/11013031/diff/1/chrome/browser/memory_details.cc
File chrome/browser/memory_details.cc (right):

https://codereview.chromium.org/11013031/diff/1/chrome/browser/memory_details...
chrome/browser/memory_details.cc:232: if (extension_service != NULL)
On 2012/10/03 21:00:13, John Abd-El-Malek wrote:
> nit: chrome style is to skip the "!= NULL"

Done.

https://codereview.chromium.org/11013031/diff/1/chrome/browser/memory_details...
chrome/browser/memory_details.cc:255: } else if (extension_process_map != NULL
&&
On 2012/10/03 21:00:13, John Abd-El-Malek wrote:
> ditto (also below)

Done.

[whywhat, 2012-10-04 14:16:15]

Seems tricky to add a test:
- ContentShell doesn't load memory details, i.e. loading chrome://memory in the
shell doesn't call the methods I want to test
- browsertest would have extension service since it's not for Android (yet?)
- unittest would require refactoring the function since it's checking it's
running on UI thread and it uses some browser level singletons

Seems like a unittest and moving part of CollectChildInfoOnUIThread into a
smaller testable method is the way to go.

[whywhat, 2012-10-04 18:24:24]

Added a unit test. PTAL!

[jam, 2012-10-04 22:11:44]

hmm, i'm not sure how valuable this test is. i.e. it might catch this very
specific use of a null pointer in this function. but there are so many places
that call it. at the extreme, all that code will have to be refactored so it's
called in a unittest? somehow i don't think that'll help readability. as such
i'm apprehensive of a unittest like this (as opposed to when android has more
integration tests that launch the task manager)

[whywhat, 2012-10-05 08:23:50]

On 2012/10/04 22:11:44, John Abd-El-Malek wrote:
> hmm, i'm not sure how valuable this test is. i.e. it might catch this very
> specific use of a null pointer in this function. but there are so many places
> that call it. at the extreme, all that code will have to be refactored so it's
> called in a unittest? somehow i don't think that'll help readability. as such
> i'm apprehensive of a unittest like this (as opposed to when android has more
> integration tests that launch the task manager)

I agree that having an integration test that would crash on any null pointer use
would save from more errors that just in this function.  I'll see if I can
something like it downstream. The test I've added probably protects from a)
deleting my change ignoring the comment about Android and b) introducing a new
use of ExtensionService without checking it for NULL. Both events seem pretty
unlikely to me.

I have however a couple of arguments for the unittest:
1) this code doesn't have any tests at all and for me it seems easier to add
test cases to an existing test than writing a new test from scratch; so when
this code will change in the future the author will add tests for it and more
test coverage is better
2) writing code that's easier to test usually produces more readable and
reusable code; the function in question occupied two screens on my 24"
vertically oriented monitor, not it's logically split into two, each fits on a
single screen; I believe it can be refactored better/further but wanted to get
your approval of my approach first.

[whywhat, 2012-10-08 16:14:42]

Soft ping + Avi in case John is away

[jam, 2012-10-10 03:39:52]

On 2012/10/05 08:23:50, whywhat wrote:
> On 2012/10/04 22:11:44, John Abd-El-Malek wrote:
> > hmm, i'm not sure how valuable this test is. i.e. it might catch this very
> > specific use of a null pointer in this function. but there are so many
places
> > that call it. at the extreme, all that code will have to be refactored so
it's
> > called in a unittest? somehow i don't think that'll help readability. as
such
> > i'm apprehensive of a unittest like this (as opposed to when android has
more
> > integration tests that launch the task manager)
> 
> I agree that having an integration test that would crash on any null pointer
use
> would save from more errors that just in this function.  I'll see if I can
> something like it downstream. The test I've added probably protects from a)
> deleting my change ignoring the comment about Android and b) introducing a new
> use of ExtensionService without checking it for NULL. Both events seem pretty
> unlikely to me.
> 
> I have however a couple of arguments for the unittest:
> 1) this code doesn't have any tests at all and for me it seems easier to add
> test cases to an existing test than writing a new test from scratch; so when
> this code will change in the future the author will add tests for it and more
> test coverage is better

this is not a test in the sense that it checks expected vs actual results. it's
just calling a function to make sure it doesn't crash. it seems that taking this
argument, one can write many tests for the code to call all the different
functions to ensure they don't crash. i don't think that would make our testing
situation better (even if it would increase a code coverage metric).

> 2) writing code that's easier to test usually produces more readable and
> reusable code; the function in question occupied two screens on my 24"
> vertically oriented monitor, not it's logically split into two, each fits on a
> single screen; I believe it can be refactored better/further but wanted to get
> your approval of my approach first.

[whywhat, 2012-10-10 09:05:36]

Thanks for discussing this further.
Undid the test and method refactoring.

[jam, 2012-10-11 15:58:42]

lgtm

[whywhat, 2012-10-11 16:01:13]

On 2012/10/11 15:58:42, John Abd-El-Malek wrote:
> lgtm

Thanks for reviewing!

[commit-bot: I haz the power, 2012-10-11 16:01:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avayvod@chromium.org/11013031/24003

[commit-bot: I haz the power, 2012-10-11 18:04:28]

Change committed as 161374