New issue
Advanced search Search tips

Issue 747224 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Devtools protocol: Page.navigate result implementation does not match protocol description

Reported by r.charle...@gmail.com, Jul 21 2017

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/603.2.4 (KHTML, like Gecko) Version/10.1.1 Safari/603.2.4

Steps to reproduce the problem:
1. In a virtualenv with Python 3.6, run `pip install chromewhip`
2. In another shell, run `/Applications/Google\ Chrome\ Canary.app/Contents/MacOS/Google\ Chrome\ Canary --remote-debugging-port=9222`
3. Run the following script
```
import asyncio
import logging

from chromewhip import Chrome
from chromewhip.protocol import page, dom

# see logging from chromewhip
logging.basicConfig(level=logging.DEBUG)

HOST = '127.0.0.1'
PORT = 9222

loop = asyncio.get_event_loop()
c = Chrome(host=HOST, port=PORT)

loop.run_until_complete(c.connect())

tab = c.tabs[0]

loop.run_until_complete(tab.enable_page_events())

cmd = page.Page.navigate(url='http://nzherald.co.nz')

# send_command will return once the frameStoppedLoading event is received THAT matches
# the frameId that it is in the returned command payload.
await_on_event_type = page.FrameStoppedLoadingEvent

result = loop.run_until_complete(tab.send_command(cmd, await_on_event_type))
```
3. read the logs - `send_command` will timeout because frameId is incorrect.
4. run the script again, `send_command` will succeed this time

What is the expected behavior?
Running the script only once should result in no timeout, because frameId is correct.

What went wrong?
See attached file for relevant websocket messages that arise from running the script.

According to latest protocol documentation https://chromedevtools.github.io/devtools-protocol/tot/Page/#method-navigate, returned frameId should be the one being navigated. This does not match the behaviour, where it returns an old/soon to be discarded frameId.

Did this work before? N/A 

Does this work in other browsers? Yes

Chrome version: 61.0.3162.0  Channel: canary
OS Version: OS X 10.12.5
Flash Version:
 
devtools-messages.txt
663 bytes View Download
Components: Platform>DevTools>Platform
Labels: -Hotlist-Interop
Cc: kkaluri@chromium.org
Labels: TE-NeedsTriageHelp
Unable to triage this issue from TE-End, adding "TE-NeedsTriageHelp" for further triage from dev team.

Comment 3 by caseq@chromium.org, Jul 24 2017

Owner: dgozman@chromium.org
Status: Assigned (was: Unconfirmed)
Labels: -Type-Bug -Pri-2 -OS-Mac OS-All Pri-1 Type-Bug-Regression
This does repro for me in 62.0.3166.0 with PlzNavigate enabled.


Project Member

Comment 5 by bugdroid1@chromium.org, Jul 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/40d4ce0e6ae34044b4c8a13b571d2454b187ac05

commit 40d4ce0e6ae34044b4c8a13b571d2454b187ac05
Author: Dmitry Gozman <dgozman@chromium.org>
Date: Thu Jul 27 23:27:11 2017

[DevTools] Suspend Page.navigate command until navigation has finished

With browser side navigation, WCO::DidStartNavigation does not happen
synchronously after calling NavigationController::LoadURL(), which makes
Page.navigate command to be dispatched to old renderer during cross-process
navigations and yields a stale frame id.

This patch specifically works around the problem for Page.navigate
by calling back synchronously to RFDTAH.

BUG= 747224 

Change-Id: I44935a407a4223004ef9873a49fc18dabbbfe02a
Reviewed-on: https://chromium-review.googlesource.com/585653
Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490096}
[modify] https://crrev.com/40d4ce0e6ae34044b4c8a13b571d2454b187ac05/content/browser/devtools/protocol/devtools_protocol_browsertest.cc
[modify] https://crrev.com/40d4ce0e6ae34044b4c8a13b571d2454b187ac05/content/browser/devtools/render_frame_devtools_agent_host.cc
[modify] https://crrev.com/40d4ce0e6ae34044b4c8a13b571d2454b187ac05/content/browser/devtools/render_frame_devtools_agent_host.h

Project Member

Comment 6 by bugdroid1@chromium.org, Jul 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/23f4680286ad33ed59f24303954ff5ab79ae6cf8

commit 23f4680286ad33ed59f24303954ff5ab79ae6cf8
Author: meade_UTC10 <meade@chromium.org>
Date: Fri Jul 28 01:47:03 2017

Revert "[DevTools] Suspend Page.navigate command until navigation has finished"

This reverts commit 40d4ce0e6ae34044b4c8a13b571d2454b187ac05.

Reason for revert: Seems to break browser_side_navigation_content_browsertests
on at least two mac builders (see  http://crbug.com/749915 )

Original change's description:
> [DevTools] Suspend Page.navigate command until navigation has finished
> 
> With browser side navigation, WCO::DidStartNavigation does not happen
> synchronously after calling NavigationController::LoadURL(), which makes
> Page.navigate command to be dispatched to old renderer during cross-process
> navigations and yields a stale frame id.
> 
> This patch specifically works around the problem for Page.navigate
> by calling back synchronously to RFDTAH.
> 
> BUG= 747224 
> 
> Change-Id: I44935a407a4223004ef9873a49fc18dabbbfe02a
> Reviewed-on: https://chromium-review.googlesource.com/585653
> Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
> Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#490096}

TBR=dgozman@chromium.org,pfeldman@chromium.org

Change-Id: Idad579478dd29cb59d35b688f4ad9b64628ec5fe
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  747224 , 749915 
Reviewed-on: https://chromium-review.googlesource.com/590509
Reviewed-by: meade_UTC10 <meade@chromium.org>
Commit-Queue: meade_UTC10 <meade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490191}
[modify] https://crrev.com/23f4680286ad33ed59f24303954ff5ab79ae6cf8/content/browser/devtools/protocol/devtools_protocol_browsertest.cc
[modify] https://crrev.com/23f4680286ad33ed59f24303954ff5ab79ae6cf8/content/browser/devtools/render_frame_devtools_agent_host.cc
[modify] https://crrev.com/23f4680286ad33ed59f24303954ff5ab79ae6cf8/content/browser/devtools/render_frame_devtools_agent_host.h

Project Member

Comment 7 by bugdroid1@chromium.org, Jul 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2fe47b35464eeca0218e373fbea01cb20ca0e449

commit 2fe47b35464eeca0218e373fbea01cb20ca0e449
Author: Dmitry Gozman <dgozman@chromium.org>
Date: Fri Jul 28 23:59:53 2017

Reland "[DevTools] Suspend Page.navigate command until navigation has finished"

With browser side navigation, WCO::DidStartNavigation does not happen
synchronously after calling NavigationController::LoadURL(), which makes
Page.navigate command to be dispatched to old renderer during cross-process
navigations and yields a stale frame id.

This patch specifically works around the problem for Page.navigate
by calling back synchronously to RFDTAH.

BUG= 747224 

Reviewed-on: https://chromium-review.googlesource.com/585653
Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#490096}
Change-Id: I5f83b75e44e21daea5a7b958263ae6119f6dbc99
Reviewed-on: https://chromium-review.googlesource.com/592149
Cr-Commit-Position: refs/heads/master@{#490546}
[modify] https://crrev.com/2fe47b35464eeca0218e373fbea01cb20ca0e449/content/browser/devtools/protocol/devtools_protocol_browsertest.cc
[modify] https://crrev.com/2fe47b35464eeca0218e373fbea01cb20ca0e449/content/browser/devtools/render_frame_devtools_agent_host.cc
[modify] https://crrev.com/2fe47b35464eeca0218e373fbea01cb20ca0e449/content/browser/devtools/render_frame_devtools_agent_host.h

Labels: M-61 ReleaseBlock-Stable
Labels: Merge-Request-61
Status: Started (was: Assigned)
This is live on Canary for a week, and nothing seems broken. Requesting merge to M61.
Project Member

Comment 10 by sheriffbot@chromium.org, Aug 7 2017

Labels: -Merge-Request-61 Merge-Review-61 Hotlist-Merge-Review
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-61 Merge-Approved-61
Approving merge to M61 branch 3163 based on comment #9 and per chat with dgozman@. Please merge ASAP. Thank you.
Project Member

Comment 12 by bugdroid1@chromium.org, Aug 8 2017

Labels: -merge-approved-61 merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f3a99c2dfc7219d6cc67f7b32f8a76629b621697

commit f3a99c2dfc7219d6cc67f7b32f8a76629b621697
Author: Dmitry Gozman <dgozman@chromium.org>
Date: Tue Aug 08 01:32:11 2017

Merge to 3163 "Reland "[DevTools] Suspend Page.navigate command until navigation has finished""

With browser side navigation, WCO::DidStartNavigation does not happen
synchronously after calling NavigationController::LoadURL(), which makes
Page.navigate command to be dispatched to old renderer during cross-process
navigations and yields a stale frame id.

This patch specifically works around the problem for Page.navigate
by calling back synchronously to RFDTAH.

BUG= 747224 
TBR=dgozman@chromium.org, pfeldman@chromium.org

(cherry picked from commit 2fe47b35464eeca0218e373fbea01cb20ca0e449)

Reviewed-on: https://chromium-review.googlesource.com/585653
Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Cr-Original-Original-Commit-Position: refs/heads/master@{#490096}
Change-Id: I5f83b75e44e21daea5a7b958263ae6119f6dbc99
Reviewed-on: https://chromium-review.googlesource.com/592149
Cr-Original-Commit-Position: refs/heads/master@{#490546}
Reviewed-on: https://chromium-review.googlesource.com/604989
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#375}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/f3a99c2dfc7219d6cc67f7b32f8a76629b621697/content/browser/devtools/protocol/devtools_protocol_browsertest.cc
[modify] https://crrev.com/f3a99c2dfc7219d6cc67f7b32f8a76629b621697/content/browser/devtools/render_frame_devtools_agent_host.cc
[modify] https://crrev.com/f3a99c2dfc7219d6cc67f7b32f8a76629b621697/content/browser/devtools/render_frame_devtools_agent_host.h

Status: Fixed (was: Started)

Sign in to add a comment