Make ScriptBadgeController record when extensions run scripts on pages yet to

Make ScriptBadgeController record when extensions run scripts on pages yet to
be navigated to (when IPCs for content script injection arrive before the page
ID is updated, whenever that is).

I'm a little surprised this can happen, but we're getting events for scripts
when there is no page ID yet, implying that it can. This fixes bug 138323 in a
general sense.

BUG=138323

[Jeffrey Yasskin, 2012-07-25 22:22:39]

http://codereview.chromium.org/10822007/diff/8001/chrome/browser/extensions/s...
File chrome/browser/extensions/script_badge_controller.cc (right):

http://codereview.chromium.org/10822007/diff/8001/chrome/browser/extensions/s...
chrome/browser/extensions/script_badge_controller.cc:102: if (on_page_id <
current_page_id)
Is there documentation that the page id monotonically increases? I don't see it
in navigation_entry.h.

http://codereview.chromium.org/10822007/diff/8001/chrome/browser/extensions/s...
File chrome/browser/extensions/script_badge_controller.h (right):

http://codereview.chromium.org/10822007/diff/8001/chrome/browser/extensions/s...
chrome/browser/extensions/script_badge_controller.h:73: // Public to be called
from tests.
<obligatory-c++-trivia>
Because these implement public methods from public base classes, you can
actually call them from tests using
"static_cast<content::WebContentsObserver*>(script_badge_controller)->OnContentScriptsExecuting(...)".
That defeats the general point of making them private, although it might serve
enough documentation purpose to be worth privatizing the methods anyway.
</obligatory-c++-trivia>

http://codereview.chromium.org/10822007/diff/8001/chrome/browser/extensions/s...
chrome/browser/extensions/script_badge_controller.h:120: // Extensions that have
actions in a page ID that hasn't been reached yet.
This feels like something that will bite us later if we don't figure out what's
really causing it. How does the OnExecuting message get received before the
navigation entry is set?

http://codereview.chromium.org/10822007/diff/8001/chrome/browser/extensions/s...
File chrome/browser/extensions/script_badge_controller_unittest.cc (left):

http://codereview.chromium.org/10822007/diff/8001/chrome/browser/extensions/s...
chrome/browser/extensions/script_badge_controller_unittest.cc:200: .Set("name",
"Extension")
I'm not a fan of moving all of the extension creations into a common function.
Doing that makes it unclear which attributes of the single manifest are relevant
to which test.

http://codereview.chromium.org/10822007/diff/8001/chrome/browser/extensions/s...
File chrome/browser/extensions/script_badge_controller_unittest.cc (right):

http://codereview.chromium.org/10822007/diff/8001/chrome/browser/extensions/s...
chrome/browser/extensions/script_badge_controller_unittest.cc:105:
notification_tracker.ListenFor(
Hey cool, that's better than my class. :)

http://codereview.chromium.org/10822007/diff/8001/chrome/browser/extensions/s...
chrome/browser/extensions/script_badge_controller_unittest.cc:144: // Extension
calls executeScript() on a page that the browser doesn't
This seems redundant with the ContentScripts test. Can you either extract the
common code inside ScriptBadgeController so you only need to test it once, or
refactor the tests so that you can run the same code twice with different
notification callbacks?

[not at google - send to devlin, 2012-07-26 08:52:45]

Didn't get time to actually finish updating the test, but sending out some
replies that hopefully jam can have a look at.

Background: we have "script badges" for extensions, which indicate that an
extension is active on a page. They that show up in the location bar (next to
the star), there is a flag for them in about:flags.

They're a security feature, so it's important that they accurately reflect
what's in the page. We have the renderer IPCing the browser when an extension
executes a script (content scripts or tabs.executeScript) which comes into
extensions::ScriptBadgeController, when then displays them in the location bar.

In order to avoid races, the renderer sends up the page ID which
ScriptBadgeController compares to navigation controller's page ID. However, it
looks like the navigation entry can be NULL, see http://crbug.com/138323 (the
bug this patch is trying to address). Under what circumstances could that
happen? We need to do something sensible, e.g. if it's NULL because the
navigation controller hasn't caught up (somehow) then we need to buffer these
IPCs until it has.

So yeah, there are questions there in the code. I'll have a go at digging deeper
into content to see what this is (or maybe it's something much simpler, in which
case... cool).

http://codereview.chromium.org/10822007/diff/8001/chrome/browser/extensions/s...
File chrome/browser/extensions/script_badge_controller.cc (right):

http://codereview.chromium.org/10822007/diff/8001/chrome/browser/extensions/s...
chrome/browser/extensions/script_badge_controller.cc:102: if (on_page_id <
current_page_id)
On 2012/07/25 22:22:39, Jeffrey Yasskin wrote:
> Is there documentation that the page id monotonically increases? I don't see
it
> in navigation_entry.h.

Yeah, I don't know. @jam do you know?

http://codereview.chromium.org/10822007/diff/8001/chrome/browser/extensions/s...
File chrome/browser/extensions/script_badge_controller.h (right):

http://codereview.chromium.org/10822007/diff/8001/chrome/browser/extensions/s...
chrome/browser/extensions/script_badge_controller.h:73: // Public to be called
from tests.
On 2012/07/25 22:22:39, Jeffrey Yasskin wrote:
> <obligatory-c++-trivia>
> Because these implement public methods from public base classes, you can
> actually call them from tests using
>
"static_cast<content::WebContentsObserver*>(script_badge_controller)->OnContentScriptsExecuting(...)".
> That defeats the general point of making them private, although it might serve
> enough documentation purpose to be worth privatizing the methods anyway.
> </obligatory-c++-trivia>

I was going to say "I like it" and started changing it.

Then I was like, damn it past-Ben, you forgot to make that method virtual, lazy.

Then I realised it's not an override, just one of those methods that you
delegate IPC handling to. So, public it is :(

(either that or faking out IPCs, but tooo much effort).

http://codereview.chromium.org/10822007/diff/8001/chrome/browser/extensions/s...
chrome/browser/extensions/script_badge_controller.h:120: // Extensions that have
actions in a page ID that hasn't been reached yet.
On 2012/07/25 22:22:39, Jeffrey Yasskin wrote:
> This feels like something that will bite us later if we don't figure out
what's
> really causing it. How does the OnExecuting message get received before the
> navigation entry is set?

Yeah, so the crash was from OnContentScriptExecuting.

My initial assumption, and the assumption of this patch, is that it runs early
(like on document creation) then send up the IPC, somehow before the navigation
entry is set.

The documentation in NavigationController just says "it can be NULL" but not
why. I assumed it was before there were any navigation entries? And if these
IPCs can arrive before the navigation entry is created, then presumably they can
arrive when they're stale too?

I briefly also wondered if it was because the navigation stuff had been torn
down, but then why would the IPC arrive? Saying that now, I realise it's just as
feasible as it not being around when it's been created, so eh.

I guess I should really go and look through the content code to verify all this.
It can't be a black box forever.

http://codereview.chromium.org/10822007/diff/8001/chrome/browser/extensions/s...
File chrome/browser/extensions/script_badge_controller_unittest.cc (right):

http://codereview.chromium.org/10822007/diff/8001/chrome/browser/extensions/s...
chrome/browser/extensions/script_badge_controller_unittest.cc:105:
notification_tracker.ListenFor(
On 2012/07/25 22:22:39, Jeffrey Yasskin wrote:
> Hey cool, that's better than my class. :)

:) a lucky find. I was looking for where yours was actually, didn't realise it
was in here, found that one instead.

[jam, 2012-07-26 21:49:19]

removing myself as owner. look at extensions owners file

[not at google - send to devlin, 2012-07-27 01:13:25]

I added you as a legimate reviewer, I had a question about the code.

This OWNERS thing is silly.