Bug 174670 – WebDriver: wait until navigation is complete before running new commands and after a click

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 315976 [details]
Patch

[Build Bot]

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.

[Carlos Garcia Campos]

Doesn't build in GTK+ because it depends on patch attached to bug #174668

[Build Bot]

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

[Build Bot]

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

[Blaze Burg]

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

[Blaze Burg]

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.

[Radar WebKit Bug Importer]

<rdar://problem/33431632>

[Blaze Burg]

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?

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Committed r219722: <http://trac.webkit.org/changeset/219722>

[Blaze Burg]

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.

[Carlos Garcia Campos]

(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.

[Blaze Burg]

(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.