Bug 174670 - WebDriver: wait until navigation is complete before running new commands and after a click
Summary: WebDriver: wait until navigation is complete before running new commands and ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 174668
Blocks: 174633 174672
  Show dependency treegraph
 
Reported: 2017-07-20 03:21 PDT by Carlos Garcia Campos
Modified: 2017-07-24 11:46 PDT (History)
4 users (show)

See Also:


Attachments
Patch (30.77 KB, patch)
2017-07-20 03:37 PDT, Carlos Garcia Campos
bburg: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-elcapitan (1.76 MB, application/zip)
2017-07-20 04:53 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2017-07-20 03:21:45 PDT
We are already waiting for navigation to complete after navigation commands (go, back, forward, refresh), but the spec says we should always wait before executing a new command and also after a click. This is causing test testShouldBeAbleToNavigateBackInTheBrowserHistoryInPresenceOfIframes to simetimes fail, because it does .click() + .title and expects the title to tbe the one of the page loaded by the click. Since the load happens very fast, the test usually passes, but in a real case with a slower load, the title of the previous page will be returned most of the times.
Comment 1 Carlos Garcia Campos 2017-07-20 03:37:04 PDT
Created attachment 315976 [details]
Patch
Comment 2 Build Bot 2017-07-20 03:38:18 PDT
Attachment 315976 [details] did not pass style-queue:


ERROR: Source/WebDriver/Session.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:930:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Carlos Garcia Campos 2017-07-20 03:53:58 PDT
Doesn't build in GTK+ because it depends on patch attached to bug #174668
Comment 4 Build Bot 2017-07-20 04:53:54 PDT
Comment on attachment 315976 [details]
Patch

Attachment 315976 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4154798

New failing tests:
media/modern-media-controls/scrubber-support/scrubber-support-drag.html
Comment 5 Build Bot 2017-07-20 04:53:55 PDT
Created attachment 315981 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 6 Brian Burg 2017-07-20 10:09:22 PDT
Comment on attachment 315976 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315976&action=review

> Source/WebDriver/ChangeLog:10
> +        testShouldBeAbleToNavigateBackInTheBrowserHistoryInPresenceOfIframes to simetimes fail, because it does .click()

Nit: sometimes
Comment 7 Brian Burg 2017-07-20 10:48:42 PDT
Comment on attachment 315976 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315976&action=review

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:385
> +

I'll hold off on being pedantic about spec conformance until we fix the FIXME. When you implement the page load strategy and the rest of the steps in §9, please include the steps in comments here and in the private methods so it's easy to verify we do the right thing.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:400
> +    if (auto callback = m_pendingNavigationInBrowsingContextCallbacksPerPage.take(page.pageID()))

Note: per §6.3, substeps 4.3.1–4.3.4, the spec assumes commands are serialized into a queue per session, otherwise one command could cause another one to timeout if they both need to wait for navigation to complete.

I don't think it's the case right now that we queue commands explicitly or implicitly in WebKitDriver, nor in safaridriver. So this could hypothetically be a problem right now, but it seems unlikely in practice.
Comment 8 Radar WebKit Bug Importer 2017-07-20 10:48:51 PDT
<rdar://problem/33431632>
Comment 9 Brian Burg 2017-07-20 11:37:29 PDT
Comment on attachment 315976 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315976&action=review

r=me with a few questions. Great work!

> Source/WebDriver/ChangeLog:15
> +        6.3 Processing Model. Step 7. Wait for navigation to complete. If this returns an error return its value and

Nit: step 6

This suggests that we should be waiting for navigations to complete after any command. Should this be extracted out of particular Session methods, then?

> Source/WebDriver/ChangeLog:19
> +        14.1 Element Click. Step 10. If the click causes navigation: 1. Run the post-navigation checks and return its

I don't understand substep 1. How could the post-navigation checks do anything useful if the click just initiated a load? Ostensibly, the load has likely not been sent out yet, or hasn't received a response in the time waited during Step 9.

> Source/WebDriver/Session.cpp:947
> +    if (!m_toplevelBrowsingContext) {

Nit: add quotations from specific sections/steps of the spec, especially for error cases.

> Source/WebDriver/Session.cpp:954
> +    if (m_browsingContext)

Can we rename this local to m_selectedFrameBrowsingContext or something similar? Otherwise it's a bit obscure why there are two browsing context members.

> Source/WebKit/ChangeLog:24
> +        all the frames. Complete page operations of it's a main frame, or frame operations otherwise.

Nit: of->if

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:467
> +        if (auto callback = m_pendingNavigationInBrowsingContextCallbacksPerPage.take(frame.page()->pageID()))

When asked to wait on a page navigation, couldn't we use the main frame as the callback key and remove m_pendingNavigationInBrowsingContextCallbacksPerPage? Or does the frame somehow not survive long enough?
Comment 10 Carlos Garcia Campos 2017-07-20 23:45:51 PDT
Comment on attachment 315976 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315976&action=review

>> Source/WebDriver/ChangeLog:15
>> +        6.3 Processing Model. Step 7. Wait for navigation to complete. If this returns an error return its value and
> 
> Nit: step 6
> 
> This suggests that we should be waiting for navigations to complete after any command. Should this be extracted out of particular Session methods, then?

hmm, are you using an old version of the spec or something? This is Section 6.3 and the step is indeed 7. And it suggests we should be waiting for navigations *before* any command, step 8 is "Let response result be the return value obtained by running the remote end steps for command with url variables as arguments." I added this for some of the commands only, though, I don't think we want to wait for any navigation to get the window handles, for example, but we want for sure before getting the page title.

>> Source/WebDriver/ChangeLog:19
>> +        14.1 Element Click. Step 10. If the click causes navigation: 1. Run the post-navigation checks and return its
> 
> I don't understand substep 1. How could the post-navigation checks do anything useful if the click just initiated a load? Ostensibly, the load has likely not been sent out yet, or hasn't received a response in the time waited during Step 9.

Yeah, I don't understand it either. I guess it assumes single process browsers in which case the load could have started right after the click. Ah, I missed step 9, maybe that ensures the load has started right after the click (in case there's a load) and we can add the wait there like we do for navigation commands, instead of doing it in the driver, reducing the possibilities of race condition.

>> Source/WebDriver/Session.cpp:954
>> +    if (m_browsingContext)
> 
> Can we rename this local to m_selectedFrameBrowsingContext or something similar? Otherwise it's a bit obscure why there are two browsing context members.

I think the spec uses top level browsing context and current browsing context, so maybe m_currentBrowsingContext? or maybe using frame explicitly makes the thing easier. I'll do the rename in a follow up patch in any case.

>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:400
>> +    if (auto callback = m_pendingNavigationInBrowsingContextCallbacksPerPage.take(page.pageID()))
> 
> Note: per §6.3, substeps 4.3.1–4.3.4, the spec assumes commands are serialized into a queue per session, otherwise one command could cause another one to timeout if they both need to wait for navigation to complete.
> 
> I don't think it's the case right now that we queue commands explicitly or implicitly in WebKitDriver, nor in safaridriver. So this could hypothetically be a problem right now, but it seems unlikely in practice.

Here step 4 doesn't have substeps. 4. Let session id be the corresponding variable from url variables.

>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:467
>> +        if (auto callback = m_pendingNavigationInBrowsingContextCallbacksPerPage.take(frame.page()->pageID()))
> 
> When asked to wait on a page navigation, couldn't we use the main frame as the callback key and remove m_pendingNavigationInBrowsingContextCallbacksPerPage? Or does the frame somehow not survive long enough?

We might not have a main frame yet right after a navigation command, it's created in WebPageProxy::didCreateMainFrame, so we wouldn't have an id to use as the key when adding it to the table.
Comment 11 Carlos Garcia Campos 2017-07-21 02:50:59 PDT
Committed r219722: <http://trac.webkit.org/changeset/219722>
Comment 12 Brian Burg 2017-07-21 11:17:56 PDT
Comment on attachment 315976 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315976&action=review

>>> Source/WebDriver/ChangeLog:15
>>> +        6.3 Processing Model. Step 7. Wait for navigation to complete. If this returns an error return its value and
>> 
>> Nit: step 6
>> 
>> This suggests that we should be waiting for navigations to complete after any command. Should this be extracted out of particular Session methods, then?
> 
> hmm, are you using an old version of the spec or something? This is Section 6.3 and the step is indeed 7. And it suggests we should be waiting for navigations *before* any command, step 8 is "Let response result be the return value obtained by running the remote end steps for command with url variables as arguments." I added this for some of the commands only, though, I don't think we want to wait for any navigation to get the window handles, for example, but we want for sure before getting the page title.

I asked the spec editors, and they haven't updated the W3C copy of the CR in months. You should refer to the latest editor draft here. https://w3c.github.io/webdriver/webdriver-spec.html

They said they'll be updating the one at w3.org next week, and that section numbers are stabilized now (in the editor draft).

Sorry for the confusion! The w3.org copy used to sync on a regular basis.

>>> Source/WebDriver/Session.cpp:954
>>> +    if (m_browsingContext)
>> 
>> Can we rename this local to m_selectedFrameBrowsingContext or something similar? Otherwise it's a bit obscure why there are two browsing context members.
> 
> I think the spec uses top level browsing context and current browsing context, so maybe m_currentBrowsingContext? or maybe using frame explicitly makes the thing easier. I'll do the rename in a follow up patch in any case.

Ok. Either way is better than current.

>>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:400
>>> +    if (auto callback = m_pendingNavigationInBrowsingContextCallbacksPerPage.take(page.pageID()))
>> 
>> Note: per §6.3, substeps 4.3.1–4.3.4, the spec assumes commands are serialized into a queue per session, otherwise one command could cause another one to timeout if they both need to wait for navigation to complete.
>> 
>> I don't think it's the case right now that we queue commands explicitly or implicitly in WebKitDriver, nor in safaridriver. So this could hypothetically be a problem right now, but it seems unlikely in practice.
> 
> Here step 4 doesn't have substeps. 4. Let session id be the corresponding variable from url variables.

See above comment about spec version.

>>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:467
>>> +        if (auto callback = m_pendingNavigationInBrowsingContextCallbacksPerPage.take(frame.page()->pageID()))
>> 
>> When asked to wait on a page navigation, couldn't we use the main frame as the callback key and remove m_pendingNavigationInBrowsingContextCallbacksPerPage? Or does the frame somehow not survive long enough?
> 
> We might not have a main frame yet right after a navigation command, it's created in WebPageProxy::didCreateMainFrame, so we wouldn't have an id to use as the key when adding it to the table.

Gotcha, I must have forgotten fixing that.
Comment 13 Carlos Garcia Campos 2017-07-23 01:57:32 PDT
(In reply to Brian Burg from comment #12)
> Comment on attachment 315976 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315976&action=review
> 
> >>> Source/WebDriver/ChangeLog:15
> >>> +        6.3 Processing Model. Step 7. Wait for navigation to complete. If this returns an error return its value and
> >> 
> >> Nit: step 6
> >> 
> >> This suggests that we should be waiting for navigations to complete after any command. Should this be extracted out of particular Session methods, then?
> > 
> > hmm, are you using an old version of the spec or something? This is Section 6.3 and the step is indeed 7. And it suggests we should be waiting for navigations *before* any command, step 8 is "Let response result be the return value obtained by running the remote end steps for command with url variables as arguments." I added this for some of the commands only, though, I don't think we want to wait for any navigation to get the window handles, for example, but we want for sure before getting the page title.
> 
> I asked the spec editors, and they haven't updated the W3C copy of the CR in
> months. You should refer to the latest editor draft here.
> https://w3c.github.io/webdriver/webdriver-spec.html

Oh, I had no idea :-( I assumed w3c was up to date. Now I understand the new request queue thing you were talking about. But I don't understand the spec, though, so we queue the request, wait for it to reach the queue and then we dequeue it?

> They said they'll be updating the one at w3.org next week, and that section
> numbers are stabilized now (in the editor draft).
> 
> Sorry for the confusion! The w3.org copy used to sync on a regular basis.

No problem, my only concern are the urls to the spec we have already added to the doc, we will have to check them all.

> >>> Source/WebDriver/Session.cpp:954
> >>> +    if (m_browsingContext)
> >> 
> >> Can we rename this local to m_selectedFrameBrowsingContext or something similar? Otherwise it's a bit obscure why there are two browsing context members.
> > 
> > I think the spec uses top level browsing context and current browsing context, so maybe m_currentBrowsingContext? or maybe using frame explicitly makes the thing easier. I'll do the rename in a follow up patch in any case.
> 
> Ok. Either way is better than current.
> 
> >>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:400
> >>> +    if (auto callback = m_pendingNavigationInBrowsingContextCallbacksPerPage.take(page.pageID()))
> >> 
> >> Note: per §6.3, substeps 4.3.1–4.3.4, the spec assumes commands are serialized into a queue per session, otherwise one command could cause another one to timeout if they both need to wait for navigation to complete.
> >> 
> >> I don't think it's the case right now that we queue commands explicitly or implicitly in WebKitDriver, nor in safaridriver. So this could hypothetically be a problem right now, but it seems unlikely in practice.
> > 
> > Here step 4 doesn't have substeps. 4. Let session id be the corresponding variable from url variables.
> 
> See above comment about spec version.

Now that I've read the spec, I don't think that's a problem, because the commands are enqueued and dequeued before the wait, so I don't think we can have both waiting at the same time.

> >>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:467
> >>> +        if (auto callback = m_pendingNavigationInBrowsingContextCallbacksPerPage.take(frame.page()->pageID()))
> >> 
> >> When asked to wait on a page navigation, couldn't we use the main frame as the callback key and remove m_pendingNavigationInBrowsingContextCallbacksPerPage? Or does the frame somehow not survive long enough?
> > 
> > We might not have a main frame yet right after a navigation command, it's created in WebPageProxy::didCreateMainFrame, so we wouldn't have an id to use as the key when adding it to the table.
> 
> Gotcha, I must have forgotten fixing that.
Comment 14 Brian Burg 2017-07-24 11:46:36 PDT
(In reply to Carlos Garcia Campos from comment #13)
> (In reply to Brian Burg from comment #12)
> > Comment on attachment 315976 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=315976&action=review
> > 
> > >>> Source/WebDriver/ChangeLog:15
> > >>> +        6.3 Processing Model. Step 7. Wait for navigation to complete. If this returns an error return its value and
> > >> 
> > >> Nit: step 6
> > >> 
> > >> This suggests that we should be waiting for navigations to complete after any command. Should this be extracted out of particular Session methods, then?
> > > 
> > > hmm, are you using an old version of the spec or something? This is Section 6.3 and the step is indeed 7. And it suggests we should be waiting for navigations *before* any command, step 8 is "Let response result be the return value obtained by running the remote end steps for command with url variables as arguments." I added this for some of the commands only, though, I don't think we want to wait for any navigation to get the window handles, for example, but we want for sure before getting the page title.
> > 
> > I asked the spec editors, and they haven't updated the W3C copy of the CR in
> > months. You should refer to the latest editor draft here.
> > https://w3c.github.io/webdriver/webdriver-spec.html
> 
> Oh, I had no idea :-( I assumed w3c was up to date. Now I understand the new
> request queue thing you were talking about. But I don't understand the spec,
> though, so we queue the request, wait for it to reach the queue and then we
> dequeue it?

Basically, multiple clients could send REST API requests to the same session concurrently. The command queue forces these to be handled serially in FIFO order. Obviously, if you only use one serial queue to handle incoming HTTP requests, then there's nothing to do. However, most web servers use concurrent queues/threads, so you may need to do some queueing.