Bug 174633 – Web Automation: implicit navigations don't cause browsing context switch

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

I've noticed this debugging testShouldFocusOnTheReplacementWhenAFrameFollowsALinkToA_TopTargetedPage. The situation is something like this:

1. a frame set is loaded
2. switch frame to any frame in the set
3. get a link from the subframe and click it
4. a new page is loaded in the main frame
5. find an element that is in the new page

from 3 to 4 there's actually a browsing context switch, but web driver doesn't know about it. The subrame from which the navigation happened is still the current browsing context, and it's still referenced in WebAutomationSession (frames are never removed from the maps) and in WebAutomationSessionProxy (the script object is still protected, and pending callbacks if any are nos dispatched, it seems we are leaking the frame in the web process). So, step 5 is still trying to find the element in the previous frame, and since the frame is still alive, the evaluateJavaScriptFunction is called, but the element is not found and returns a no such element error. This would be even more problematic if the previous page had an element matching the find command. I'm not sure how to solve this, if the frame was not leaked in the wen process, at least we could get a no such frame error, and handle it as special case, switching to top level and trying again. Something like that is done in chromium:

if (attempt == 2) {
   // Switch to main frame and retry command if subframe no longer exists.                                                                                                                 
   session->SwitchToTopFrame();
}

I don't like this approach that much, but if there's no other way, at least we should do it only when error returned is no such frame, (and there's an active browsing context, of course).

Any other ideas? Did you solve that in safaridriver somehow?

[Blaze Burg]

(In reply to Carlos Garcia Campos from comment #0)
> I've noticed this debugging
> testShouldFocusOnTheReplacementWhenAFrameFollowsALinkToA_TopTargetedPage.
> The situation is something like this:
> 
> 1. a frame set is loaded
> 2. switch frame to any frame in the set
> 3. get a link from the subframe and click it
> 4. a new page is loaded in the main frame
> 5. find an element that is in the new page
> 
> from 3 to 4 there's actually a browsing context switch, but web driver
> doesn't know about it. The subrame from which the navigation happened is
> still the current browsing context, and it's still referenced in
> WebAutomationSession (frames are never removed from the maps) and in
> WebAutomationSessionProxy (the script object is still protected, and pending
> callbacks if any are nos dispatched, it seems we are leaking the frame in
> the web process). So, step 5 is still trying to find the element in the
> previous frame, and since the frame is still alive, the
> evaluateJavaScriptFunction is called, but the element is not found and
> returns a no such element error. This would be even more problematic if the
> previous page had an element matching the find command.

Great investigation!

> I'm not sure how to
> solve this, if the frame was not leaked in the web process, at least we
> could get a no such frame error, and handle it as special case, switching to
> top level and trying again. Something like that is done in chromium:
> 
> if (attempt == 2) {
>    // Switch to main frame and retry command if subframe no longer exists.  
> 
>    session->SwitchToTopFrame();
> }
> 
> I don't like this approach that much, but if there's no other way, at least
> we should do it only when error returned is no such frame, (and there's an
> active browsing context, of course).
> 
> Any other ideas? Did you solve that in safaridriver somehow?

No we haven't solved this. This is probably the cause of many nonspecific bugs we get with unexpected "element not found" errors.

What about changing the context in WebAutomationSession::navigationOccurredForPage() if needed, and sending a notification out to the driver that the current top level context changed?

[Radar WebKit Bug Importer]

<rdar://problem/33387797>

[Carlos Garcia Campos]

We are not actually leaking frames, it's that they are kept alive in the page cache.

[Carlos Garcia Campos]

(In reply to Brian Burg from comment #1)
> (In reply to Carlos Garcia Campos from comment #0)
> > I've noticed this debugging
> > testShouldFocusOnTheReplacementWhenAFrameFollowsALinkToA_TopTargetedPage.
> > The situation is something like this:
> > 
> > 1. a frame set is loaded
> > 2. switch frame to any frame in the set
> > 3. get a link from the subframe and click it
> > 4. a new page is loaded in the main frame
> > 5. find an element that is in the new page
> > 
> > from 3 to 4 there's actually a browsing context switch, but web driver
> > doesn't know about it. The subrame from which the navigation happened is
> > still the current browsing context, and it's still referenced in
> > WebAutomationSession (frames are never removed from the maps) and in
> > WebAutomationSessionProxy (the script object is still protected, and pending
> > callbacks if any are nos dispatched, it seems we are leaking the frame in
> > the web process). So, step 5 is still trying to find the element in the
> > previous frame, and since the frame is still alive, the
> > evaluateJavaScriptFunction is called, but the element is not found and
> > returns a no such element error. This would be even more problematic if the
> > previous page had an element matching the find command.
> 
> Great investigation!
> 
> > I'm not sure how to
> > solve this, if the frame was not leaked in the web process, at least we
> > could get a no such frame error, and handle it as special case, switching to
> > top level and trying again. Something like that is done in chromium:
> > 
> > if (attempt == 2) {
> >    // Switch to main frame and retry command if subframe no longer exists.  
> > 
> >    session->SwitchToTopFrame();
> > }
> > 
> > I don't like this approach that much, but if there's no other way, at least
> > we should do it only when error returned is no such frame, (and there's an
> > active browsing context, of course).
> > 
> > Any other ideas? Did you solve that in safaridriver somehow?
> 
> No we haven't solved this. This is probably the cause of many nonspecific
> bugs we get with unexpected "element not found" errors.
> 
> What about changing the context in
> WebAutomationSession::navigationOccurredForPage() if needed, and sending a
> notification out to the driver that the current top level context changed?

I think we can simply clear the m_handleWebFrameMap there, because no frames can be resolved before the page loads, any frame cached there at that point is for previous page. Then, any operation on a stale frame will end up on a non such frame error.

[Carlos Garcia Campos]

Created attachment 315979 [details]
Patch

[Build Bot]

Attachment 315979 [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]

Created attachment 315980 [details]
Patch

Oops, submitted the wrong patch

[Blaze Burg]

Comment on attachment 315980 [details]
Patch

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

r+ with change

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:468
> +        m_handleWebFrameMap.clear();

You need to clear m_webFrameHandleMap too, as they are inversed lookup tables.

[Carlos Garcia Campos]

Committed r219723: <http://trac.webkit.org/changeset/219723>