Bug 96402 – [Chromium] Fix cases where find-in-page doesn't send a final update

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Leandro Graciá Gil]

The current WebFrameImpl code has a few cases where it never sends the final find-in-page update to the embedder. This is because there are some cases where an early out prevents reportFindInPageMatchCount to be called. Furthermore, these early outs enforce a non-null size of the view which causes problems with offscreen WebViews that might be used by some external tests.

Also, we have a buggy clean-up of the active match when stopping the current find "session". With the current code we'd need two StopFinding calls, one keeping the selection and another one cleaning it in order to reset the active match. This clean-up should unconditionally happen when calling StopFinding, and it should not affect the current behaviour as it will continue finding from the current selection if any.

[Leandro Graciá Gil]

Created attachment 163405 [details]
Patch

[Adam Barth]

Comment on attachment 163405 [details]
Patch

@leandrogracia: Looks like you uploaded the wrong patch.

[Leandro Graciá Gil]

(In reply to comment #2)
> (From update of attachment 163405 [details])
> @leandrogracia: Looks like you uploaded the wrong patch.

Oops, looks like I had some git issues in my local branch. Sorry, uploading the right patch.

[Leandro Graciá Gil]

Created attachment 163653 [details]
Patch

[Adam Barth]

Comment on attachment 163653 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:8
> +        Fix some issues in the WebKit implementation that prevented to send a final
> +        reportFindInPageMatchCount message. Also, fix a buggy reset of the active match
> +        when calling the stopFinding method.

Can we add tests for these issues?  Perhaps in webkit_unit_tests?

[Leandro Graciá Gil]

(In reply to comment #5)
> (From update of attachment 163653 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163653&action=review
> 
> > Source/WebKit/chromium/ChangeLog:8
> > +        Fix some issues in the WebKit implementation that prevented to send a final
> > +        reportFindInPageMatchCount message. Also, fix a buggy reset of the active match
> > +        when calling the stopFinding method.
> 
> Can we add tests for these issues?  Perhaps in webkit_unit_tests?

I found this problem when using find-in-page with 0x0 views in test code. However, since there's no reason this shouldn't work regardless of the size, I'm removing the hasVisibleContent check. Consequently, if I write a test it won't hit that condition any more.

Still, if for some reason we have no frame or no view we still want to ensure a reply back. Do you know how I could hit these conditions in a test?

[Adam Barth]

Comment on attachment 163653 [details]
Patch

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

>>> Source/WebKit/chromium/ChangeLog:8
>>> +        when calling the stopFinding method.
>> 
>> Can we add tests for these issues?  Perhaps in webkit_unit_tests?
> 
> I found this problem when using find-in-page with 0x0 views in test code. However, since there's no reason this shouldn't work regardless of the size, I'm removing the hasVisibleContent check. Consequently, if I write a test it won't hit that condition any more.
> 
> Still, if for some reason we have no frame or no view we still want to ensure a reply back. Do you know how I could hit these conditions in a test?

Those cases occur with detached frames.  I'm not sure how we can make a detached frame in a unit test...  We have trouble testing these cases because we tend to only keep detached frames alive for a given call stack.  In LayoutTests, we typically do it by running inline script in a child frame that removes its frame from its parent using a DOM API like removeChild.  To get that working in a unit test, you'd need to them call a function that let you execute unit testing code synchronously.

I think its ok to land this patch without a test.  It's going to be a pretty fragile case to write a test for.

[WebKit Review Bot]

Comment on attachment 163653 [details]
Patch

Clearing flags on attachment: 163653

Committed r128351: <http://trac.webkit.org/changeset/128351>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

This broke many (all?) of the find in page unit tests on the linux tests canary.  See http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/23664

[James Robinson]

These broke on the mac canary as well - I suspect it's cross platform.  Example failure logs:


FindInPageControllerTest.ActivateLinkNavigatesPage: 
Killed (timed out).

FindInPageControllerTest.FindInPageEndState: 
chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:504: Failure
Value of: result.c_str()
  Actual: "{nothing focused}"
Expected: "link1"

FindInPageControllerTest.FindInPageFrames: 
chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:216: Failure
Value of: FindInPageWchar(tab, L"go", kFwd, kIgnoreCase, &ordinal)
  Actual: 0
Expected: 11
chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:219: Failure
Value of: FindInPageWchar(tab, L"goo", kFwd, kIgnoreCase, &ordinal)
  Actual: 0
Expected: 4
chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:220: Failure
Value of: ordinal
  Actual: 0
Expected: 1

[Leandro Graciá Gil]

(In reply to comment #11)
> These broke on the mac canary as well - I suspect it's cross platform.  Example failure logs:
> 
> 
> FindInPageControllerTest.ActivateLinkNavigatesPage: 
> Killed (timed out).
> 
> FindInPageControllerTest.FindInPageEndState: 
> chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:504: Failure
> Value of: result.c_str()
>   Actual: "{nothing focused}"
> Expected: "link1"
> 
> FindInPageControllerTest.FindInPageFrames: 
> chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:216: Failure
> Value of: FindInPageWchar(tab, L"go", kFwd, kIgnoreCase, &ordinal)
>   Actual: 0
> Expected: 11
> chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:219: Failure
> Value of: FindInPageWchar(tab, L"goo", kFwd, kIgnoreCase, &ordinal)
>   Actual: 0
> Expected: 4
> chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:220: Failure
> Value of: ordinal
>   Actual: 0
> Expected: 1

Looks like the change in the clean-up of the active match actually changed the behaviour and broke the tests. I'll be leaving out that part of the patch. The rest should still be fine.

[Leandro Graciá Gil]

Created attachment 163752 [details]
Patch

[Adam Barth]

Comment on attachment 163752 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 163752 [details]
Patch

Clearing flags on attachment: 163752

Committed r128409: <http://trac.webkit.org/changeset/128409>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

It seems this broken browser_tests.
http://chromegw.corp.google.com/i/chromium.webkit/builders/Linux%20Tests/builds/23673
http://chromegw.corp.google.com/i/chromium.webkit/builders/Mac10.6%20Tests/builds/13423

I'll roll this out.

[WebKit Review Bot]

Re-opened since this is blocked by 96622

[Leandro Graciá Gil]

(In reply to comment #18)
> Re-opened since this is blocked by 96622

Oh, not again... I'm starting to suspect there is a problem with the tests themselves. I tried to run them before, but my build had some kind of issue that made them fail with and without my patch.

I'll try to figure out what's happening before bothering again. Sorry for any inconveniences.

[Leandro Graciá Gil]

Created attachment 164354 [details]
Patch

[Leandro Graciá Gil]

Created attachment 164356 [details]
Patch

[Leandro Graciá Gil]

(In reply to comment #19)
> (In reply to comment #18)
> > Re-opened since this is blocked by 96622
> 
> Oh, not again... I'm starting to suspect there is a problem with the tests themselves. I tried to run them before, but my build had some kind of issue that made them fail with and without my patch.
> 
> I'll try to figure out what's happening before bothering again. Sorry for any inconveniences.

Found the issue with the previous failing patches. While conceptually correct, they were opening the door to a very subtle issue.

There is a variable called m_framesScopingCount that keeps how many frames are scoping their string matches and makes updates final when its value is 0, as we have no frames pending to finish. It happens that before resetting (starting a new find request) this variable will be 0 for the first frame because it hasn't yet counted itself. Consequently, the previous patches were sending final updates when they didn't have to scope on the first frame regardless of the existence of valid results in other frames.

We need to make sure that when starting a new find any other frames had a chance to reset (last argument in scopeStringMatches) so they can include themselves in m_framesScopingCount if required. To do this now any shouldScopeMatches failure when resetting will post a later call to itself, very much in the same way as reset works. This new call then fires the update, which will be final only if no other frames have pending scopes.

I've checked that all Chromium's FindInPageControllerTest browser tests pass after this patch.

[Leandro Graciá Gil]

Created attachment 164378 [details]
Patch

[Leandro Graciá Gil]

(In reply to comment #23)
> Created an attachment (id=164378) [details]
> Patch

This should be cleaner now as it shares the reset and finishing code with the rest of scopeStringMatches. The base functionality is still the same as in the previous patch. I've also confirmed this to pass all the Chromium's FindInPageControllerTest browser tests.

[Leandro Graciá Gil]

Created attachment 164382 [details]
Patch

[Leandro Graciá Gil]

(In reply to comment #25)
> Created an attachment (id=164382) [details]
> Patch

Minor fix: as cancelPendingScopingEffort is called unconditionally from Chromium, the m_lastFindRequestCompletedWithNoMatches flag should be reset only if the current scoping work was not completed. Tested again with Chromium's find-in-page browser tests.

[Adam Barth]

Comment on attachment 164382 [details]
Patch

Unfortunately, we don't really have someone who understands find-in-page deeply to review these patches.  What you've written above seems reasonable to me, and I'm going to rs=me this patch.

[WebKit Review Bot]

Comment on attachment 164382 [details]
Patch

Clearing flags on attachment: 164382

Committed r128784: <http://trac.webkit.org/changeset/128784>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.