WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
174633
Web Automation: implicit navigations don't cause browsing context switch
https://bugs.webkit.org/show_bug.cgi?id=174633
Summary
Web Automation: implicit navigations don't cause browsing context switch
Carlos Garcia Campos
Reported
2017-07-18 09:35:50 PDT
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?
Attachments
Patch
(30.77 KB, patch)
2017-07-20 04:40 PDT
,
Carlos Garcia Campos
no flags
Details
Formatted Diff
Diff
Patch
(2.96 KB, patch)
2017-07-20 04:44 PDT
,
Carlos Garcia Campos
bburg
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Blaze Burg
Comment 1
2017-07-18 15:08:55 PDT
(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
Comment 2
2017-07-18 15:31:36 PDT
<
rdar://problem/33387797
>
Carlos Garcia Campos
Comment 3
2017-07-20 00:09:36 PDT
We are not actually leaking frames, it's that they are kept alive in the page cache.
Carlos Garcia Campos
Comment 4
2017-07-20 00:50:58 PDT
(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
Comment 5
2017-07-20 04:40:46 PDT
Created
attachment 315979
[details]
Patch
Build Bot
Comment 6
2017-07-20 04:42:07 PDT
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
Comment 7
2017-07-20 04:44:06 PDT
Created
attachment 315980
[details]
Patch Oops, submitted the wrong patch
Blaze Burg
Comment 8
2017-07-20 11:15:25 PDT
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
Comment 9
2017-07-21 03:22:37 PDT
Committed
r219723
: <
http://trac.webkit.org/changeset/219723
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug