Fix for extension binding race condition when evaluating JS.

Fix for extension binding race condition when evaluating JS.

A bug causing extension bindings not to be initialized was fixed in
revision 155947. This patch adds a workaround for older Chrome versions.

BUG=251913
TEST=manual
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217384

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226358

[Tim Song, 2013-08-12 22:46:25]

Hi Nat, I added a fix for this extension binding race condition in Blink, and
this is the corresponding fix for telemetry. Mostly, it's just a workaround for
older versions of Chrome. PTAL.

[achuithb, 2013-08-12 23:14:45]

Re-open this CL?

https://codereview.chromium.org/19774009/diff/4001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(right):

https://codereview.chromium.org/19774009/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:217:
def use_extension_evaluate_workaround(self):
Should this be private?

[Tim Song, 2013-08-12 23:24:28]

https://codereview.chromium.org/19774009/diff/4001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(right):

https://codereview.chromium.org/19774009/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:217:
def use_extension_evaluate_workaround(self):
On 2013/08/12 23:14:45, achuith.bhandarkar wrote:
> Should this be private?

We can probably just inline this check.

[nduca, 2013-08-12 23:36:37]

lets see what dtu thinks. I'm pretty badly behind lately, so if he's happy go
for it. :)

[dtu, 2013-08-13 19:55:25]

lgtm nits

https://codereview.chromium.org/19774009/diff/4001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(right):

https://codereview.chromium.org/19774009/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:125: if
extension_page.EvaluateJavaScript("chrome.runtime == null"):
nit: single quotes

https://codereview.chromium.org/19774009/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:217:
def use_extension_evaluate_workaround(self):
On 2013/08/12 23:24:28, Tim Song wrote:
> On 2013/08/12 23:14:45, achuith.bhandarkar wrote:
> > Should this be private?
> 
> We can probably just inline this check.

+1

[Tim Song, 2013-08-13 23:07:30]

https://codereview.chromium.org/19774009/diff/4001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(right):

https://codereview.chromium.org/19774009/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:125: if
extension_page.EvaluateJavaScript("chrome.runtime == null"):
On 2013/08/13 19:55:26, Dave Tu wrote:
> nit: single quotes

Done.

https://codereview.chromium.org/19774009/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:217:
def use_extension_evaluate_workaround(self):
On 2013/08/13 19:55:26, Dave Tu wrote:
> On 2013/08/12 23:24:28, Tim Song wrote:
> > On 2013/08/12 23:14:45, achuith.bhandarkar wrote:
> > > Should this be private?
> > 
> > We can probably just inline this check.
> 
> +1

Done.

[commit-bot: I haz the power, 2013-08-13 23:13:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/19774009/16001

[commit-bot: I haz the power, 2013-08-13 23:46:09]

Change committed as 217384

[Tim Song, 2013-08-15 17:58:40]

Reopening as this patch got reverted for breaking the blink roll.

I made a mistake in testing (copied only some of the code to my blink checkout),
and didn't notice that self._chrome_branch_number isn't initialized until
PostBrowserStartupInitialization.

I'm sorry for causing this mess, and I'll write a patch that can get the version
before hand by running "chrome --version".

[Tim Song, 2013-08-30 22:20:00]

Hey everyone, because we've been running into problems with running Chrome to
get the version before launch on all platforms, I made a change to do this
workaround without needing to know the branch number. Please take another look.

[achuithb, 2013-08-30 22:24:47]

lgtm. Please wait for Dave

https://codereview.chromium.org/19774009/diff/25001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(right):

https://codereview.chromium.org/19774009/diff/25001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:142: #
Old versions of Chrome require a workaround for a race condition
this is hairy

[dtu, 2013-09-05 19:56:50]

On 2013/08/30 22:24:47, achuith.bhandarkar wrote:
> lgtm. Please wait for Dave
> 
>
https://codereview.chromium.org/19774009/diff/25001/tools/telemetry/telemetry...
> File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
> (right):
> 
>
https://codereview.chromium.org/19774009/diff/25001/tools/telemetry/telemetry...
> tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:142:
#
> Old versions of Chrome require a workaround for a race condition
> this is hairy

lgtm. Please add a TODO for when we can remove this workaround.

I agree, pretty hacky, but it's a temporary workaround for a Chrome-specific
bug, right?

[Tim Song, 2013-09-05 23:03:39]

Yeah, this bug happens in Blink, so workaround is needed until we can get the
version number before launch reliably on all platforms.

https://chromiumcodereview.appspot.com/19774009/diff/25001/tools/telemetry/te...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(right):

https://chromiumcodereview.appspot.com/19774009/diff/25001/tools/telemetry/te...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:142: #
Old versions of Chrome require a workaround for a race condition
On 2013/08/30 22:24:47, achuith.bhandarkar wrote:
> this is hairy

Done.

[achuithb, 2013-09-24 09:50:14]

Let's land this patch. autotest apis are not really usable until this works.

[Tim Song, 2013-09-24 17:56:52]

I was trying to land this patch, but there is a major issue that came up. My fix
on the Blink side returns an error if you try to evaluate JS too early. However,
this will affect regular tabs as well, and in basically everywhere in telemetry,
we call EvaluateJavaScript as soon as the tab is listed by devtools.

We could add a wait for each tab to come up, or we can add some retries to
InspectorRuntime.Evaluate if we get a "not ready" error. Otherwise, we need
quite a large refactor on the Chrome side to only list pages when they are
"ready".

[achuithb, 2013-09-24 18:59:48]

On 2013/09/24 17:56:52, Tim Song wrote:
> I was trying to land this patch, but there is a major issue that came up. My
fix
> on the Blink side returns an error if you try to evaluate JS too early.
However,
> this will affect regular tabs as well, and in basically everywhere in
telemetry,
> we call EvaluateJavaScript as soon as the tab is listed by devtools.
> 
> We could add a wait for each tab to come up, or we can add some retries to
> InspectorRuntime.Evaluate if we get a "not ready" error. Otherwise, we need
> quite a large refactor on the Chrome side to only list pages when they are
> "ready".

Does calling Reload always fix the problem?

Is there anything we could do like call Reload before the first time we need to
call EvaluateJavascript?

[Tim Song, 2013-09-25 16:59:42]

Reload calls NavigatePage, and we wait for devtools to give a response when the
page finishes loading. This process is only if we send a NavigatePage request,
so I think the problem has to do with running JavaScript on new tabs without
first navigating. I'll look into this some more.

[Tim Song, 2013-09-25 20:38:47]

I've been running this patch along with the blink-side fix on trybots, and there
is often some flakiness. eg.

http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/171024

The problem is that creating a new tab != navigating to a page. We only get a
guarantee that the page is ready by waiting for the "Page.frameNavigated"
devtools response (see inspector_page.py). 

Two potential solutions:
1. Navigate each new tab to about://blank explicitly.
2. For each new tab, poll EvaluateJavaScript until we stop getting an error.

I think (1) is a reasonable thing to do. What do you think?

[achuithb, 2013-09-25 23:53:49]

On 2013/09/25 20:38:47, Tim Song wrote:
> I've been running this patch along with the blink-side fix on trybots, and
there
> is often some flakiness. eg.
> 
> http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/171024
> 
> The problem is that creating a new tab != navigating to a page. We only get a
> guarantee that the page is ready by waiting for the "Page.frameNavigated"
> devtools response (see inspector_page.py). 
> 
> Two potential solutions:
> 1. Navigate each new tab to about://blank explicitly.
> 2. For each new tab, poll EvaluateJavaScript until we stop getting an error.
> 
> I think (1) is a reasonable thing to do. What do you think?

I'm a little confused by this.

So we're only modifying the behavior of extension background pages, not all
tabs, right? 

I thought polling EvaluateJavaScript doesn't work because once the execution
context is messed up, it doesn't ever work again? You have to reload the page,
right?

[Tim Song, 2013-09-26 17:22:08]

Because the fix is in Blink, we can't really distinguish between extension pages
and regular pages, so we return an error for all pages if you invoke
EvaluateJavaScript before the context is initialized. 

It actually doesn't really matter if the context is screwed up for regular pages
because we never bind extension apis to regular pages, so this new error is
probably a bit strict.

[Tim Song, 2013-10-01 20:20:44]

The only time we evaluate JS in a tab without first navigating in the unit
tests, so I think it's safe to commit this patch with this fix. Please take
another look.

[achuithb, 2013-10-01 23:50:39]

lgtm

https://codereview.chromium.org/19774009/diff/59001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(right):

https://codereview.chromium.org/19774009/diff/59001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:150: #
TODO(tengs): We don't have full support for getting the Chrome
Does this comment need to be updated? We neeed the workaround for chrome
regardless of version, right?

[Tim Song, 2013-10-02 00:01:20]

https://codereview.chromium.org/19774009/diff/59001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(right):

https://codereview.chromium.org/19774009/diff/59001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:150: #
TODO(tengs): We don't have full support for getting the Chrome
On 2013/10/01 23:50:39, achuith.bhandarkar wrote:
> Does this comment need to be updated? We neeed the workaround for chrome
> regardless of version, right?

If we can get the Chrome version reliably before launch, we only have to reload
the page for chrome versions without the blink patch.

[commit-bot: I haz the power, 2013-10-02 00:05:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/19774009/59001

[commit-bot: I haz the power, 2013-10-02 00:07:02]

Change committed as 226358

[achuithb, 2013-10-02 00:34:44]

https://codereview.chromium.org/19774009/diff/59001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(right):

https://codereview.chromium.org/19774009/diff/59001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:150: #
TODO(tengs): We don't have full support for getting the Chrome
On 2013/10/02 00:01:20, Tim Song wrote:
> On 2013/10/01 23:50:39, achuith.bhandarkar wrote:
> > Does this comment need to be updated? We neeed the workaround for chrome
> > regardless of version, right?
> 
> If we can get the Chrome version reliably before launch, we only have to
reload
> the page for chrome versions without the blink patch.

Ah, ok, I was under the impression that the blink patch got reverted.