[Webkit-unassigned] [Bug 174670] WebDriver: wait until navigation is complete before running new commands and after a click

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=174670

--- Comment #10 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 315976
  --> https://bugs.webkit.org/attachment.cgi?id=315976
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170721/bd2e9066/attachment.html>