Formalise a CHECK for the NULL NavigationEntry in ScriptBadgeController, and

Formalise a CHECK for the NULL NavigationEntry in ScriptBadgeController, and
add debugging for it.


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

[not at google - send to devlin, 2012-08-03 12:37:03]

Here is a preliminary step to tracking down 138323.

An alternative to http://codereview.chromium.org/10822007/ for now; I'd like to
figure this out properly, and given this stuff is still behind a flag it's ok
(we can cherry-pick out the CHECKs if necessary).

[not at google - send to devlin, 2012-08-06 06:09:42]

Hi Jeffrey,

As discussed, I've threaded through the executing URL too. It resulted in a bit
of a yak shave because there's no ROUTED6 so I needed to reduce the number of
arguments in OnExecuteScriptFinished; this was easy, just removed the "success"
parameter and made this implied by an empty error.

All of this makes me grimace a little though. Maybe it was better as it was
before, I intended for this to be a small change.

To answer your question earlier, the value of this without the URL change is
that
 - it's a CHECK so a defined crash.
 - it logs when it fails.
 - it logs the page ID that was expected. If it's 0 it implies that it was a
race starting, something else... who knows.
 - we can easily cherry-pick a CHECK-commenting-out patch.

(The URL *would* add benefit, like, maybe it's a special URL or something).

Regardless, I should actually do the buffer-on-the-stack thing but didn't have
time today. Assume, reviewing the change, that I do.

[Jeffrey Yasskin, 2012-08-06 11:39:02]

LGTM

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

http://codereview.chromium.org/10826141/diff/5001/chrome/browser/extensions/s...
chrome/browser/extensions/script_badge_controller.cc:117: "Expect a page ID of "
<< on_page_id << ", on URL \"" << on_url <<
Re "buffer on the stack", I assume you'll copy on_url and GetURL to buffers
here? I do see some check failure messages in the crash logs, so maybe it's not
necessary to force the url into the minidump? Ok with me either way.

http://codereview.chromium.org/10826141/diff/5001/chrome/browser/extensions/s...
File chrome/browser/extensions/script_executor.h (right):

http://codereview.chromium.org/10826141/diff/5001/chrome/browser/extensions/s...
chrome/browser/extensions/script_executor.h:54: // result). Success is implied
by an empty error.
Probably keep "on_page_id is only valid on success".

http://codereview.chromium.org/10826141/diff/5001/chrome/renderer/extensions/...
File chrome/renderer/extensions/extension_helper.cc (right):

http://codereview.chromium.org/10826141/diff/5001/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_helper.cc:348: routing_id(),
params.request_id, "No main frame", -1, GURL(""), val));
Yay for actual error messages. :)

[not at google - send to devlin, 2012-08-06 22:44:17]

+aa cos I'm not sure whether these IPC changes are a good idea.

[Aaron Boodman, 2012-08-07 02:08:54]

The IPC changes seem fine, but I think we just need to understand under what
conditions NavigationController::GetActiveEntry() can return NULL.

The documentation says that it can, but doesn't describe why.

Looking at the code, I guess one way it could happen is if an extension calls
executeScript() on a tab before any navigation has occurred on it. Which seems
pretty easy to do given the API. Could this explain what you're seeing?

http://codereview.chromium.org/10826141/diff/5001/chrome/renderer/extensions/...
File chrome/renderer/extensions/user_script_scheduler.cc (right):

http://codereview.chromium.org/10826141/diff/5001/chrome/renderer/extensions/...
chrome/renderer/extensions/user_script_scheduler.cc:148:
render_view->GetRoutingID(), params.request_id, "", -1, GURL(""),
This is kinda a weird response. Looks like half-successful (no error message)
and half failure (no URL-operated-on). Will this mess anything up on the browser
side?

[not at google - send to devlin, 2012-08-07 02:16:23]

How could an extension call executeScript on a tab without any navigations being
done on it? I guess in crazy-race conditions between opening a new tab and
before chrome navigates to the new tab page. That would make sense.

http://codereview.chromium.org/10826141/diff/5001/chrome/renderer/extensions/...
File chrome/renderer/extensions/user_script_scheduler.cc (right):

http://codereview.chromium.org/10826141/diff/5001/chrome/renderer/extensions/...
chrome/renderer/extensions/user_script_scheduler.cc:148:
render_view->GetRoutingID(), params.request_id, "", -1, GURL(""),
On 2012/08/07 02:08:54, Aaron Boodman wrote:
> This is kinda a weird response. Looks like half-successful (no error message)
> and half failure (no URL-operated-on). Will this mess anything up on the
browser
> side?

It was equivalently weird before; success (true) but no page ID (-1). But
shouldn't mess anything up on the browser side, no... the URL won't be valid but
callers should be checking the error, not the URL... and since I Just had to
update all call sides, pretty sure that this is what happens.

[Aaron Boodman, 2012-08-07 02:17:34]

On 2012/08/07 02:16:23, kalman wrote:
> How could an extension call executeScript on a tab without any navigations
being
> done on it? I guess in crazy-race conditions between opening a new tab and
> before chrome navigates to the new tab page. That would make sense.

chrome.tabs.onCreated.addListener(function(tab)) {

chrome.tabs.executeScript(tab.id, ...);

});

[not at google - send to devlin, 2012-08-07 02:23:56]

On 2012/08/07 02:17:34, Aaron Boodman wrote:
> On 2012/08/07 02:16:23, kalman wrote:
> > How could an extension call executeScript on a tab without any navigations
> being
> > done on it? I guess in crazy-race conditions between opening a new tab and
> > before chrome navigates to the new tab page. That would make sense.
> 
> chrome.tabs.onCreated.addListener(function(tab)) {
> 
> chrome.tabs.executeScript(tab.id, ...);
> 
> });

Makes sense... it only checks on success though. So I guess it must come in
between the renderer navigating to the page (and thus successfully executing the
script) and notifying the browser of the navigation.

Oh... but at least one of the crashes was due to *content scripts*. That's what
was especially puzzling.

[commit-bot: I haz the power, 2012-08-07 05:10:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10826141/4003

[commit-bot: I haz the power, 2012-08-07 06:25:41]

Try job failure for 10826141-4003 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-07 06:43:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10826141/5006

[commit-bot: I haz the power, 2012-08-07 08:05:05]

Change committed as 150303