[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 #13 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(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.

-- 
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/20170723/93afbc4b/attachment-0001.html>