Bug 96402 - [Chromium] Fix cases where find-in-page doesn't send a final update
Summary: [Chromium] Fix cases where find-in-page doesn't send a final update
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Leandro Graciá Gil
URL:
Keywords:
Depends on: 96573 96622
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-11 09:56 PDT by Leandro Graciá Gil
Modified: 2012-09-17 11:35 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.34 KB, patch)
2012-09-11 11:12 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (3.69 KB, patch)
2012-09-12 10:38 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (2.15 KB, patch)
2012-09-12 18:01 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (2.89 KB, patch)
2012-09-17 01:05 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (3.17 KB, patch)
2012-09-17 01:39 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (6.48 KB, patch)
2012-09-17 06:19 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (6.51 KB, patch)
2012-09-17 06:28 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leandro Graciá Gil 2012-09-11 09:56:29 PDT
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.
Comment 1 Leandro Graciá Gil 2012-09-11 11:12:25 PDT
Created attachment 163405 [details]
Patch
Comment 2 Adam Barth 2012-09-11 11:14:13 PDT
Comment on attachment 163405 [details]
Patch

@leandrogracia: Looks like you uploaded the wrong patch.
Comment 3 Leandro Graciá Gil 2012-09-12 10:32:51 PDT
(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.
Comment 4 Leandro Graciá Gil 2012-09-12 10:38:07 PDT
Created attachment 163653 [details]
Patch
Comment 5 Adam Barth 2012-09-12 11:49:24 PDT
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?
Comment 6 Leandro Graciá Gil 2012-09-12 12:21:18 PDT
(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?
Comment 7 Adam Barth 2012-09-12 12:33:14 PDT
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.
Comment 8 WebKit Review Bot 2012-09-12 13:02:03 PDT
Comment on attachment 163653 [details]
Patch

Clearing flags on attachment: 163653

Committed r128351: <http://trac.webkit.org/changeset/128351>
Comment 9 WebKit Review Bot 2012-09-12 13:02:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 James Robinson 2012-09-12 15:55:15 PDT
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
Comment 11 James Robinson 2012-09-12 16:02:48 PDT
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
Comment 12 Leandro Graciá Gil 2012-09-12 17:11:58 PDT
(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.
Comment 13 Leandro Graciá Gil 2012-09-12 18:01:56 PDT
Created attachment 163752 [details]
Patch
Comment 14 Adam Barth 2012-09-12 23:38:50 PDT
Comment on attachment 163752 [details]
Patch

ok
Comment 15 WebKit Review Bot 2012-09-12 23:58:51 PDT
Comment on attachment 163752 [details]
Patch

Clearing flags on attachment: 163752

Committed r128409: <http://trac.webkit.org/changeset/128409>
Comment 16 WebKit Review Bot 2012-09-12 23:58:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 WebKit Review Bot 2012-09-13 01:29:38 PDT
Re-opened since this is blocked by 96622
Comment 19 Leandro Graciá Gil 2012-09-13 02:18:06 PDT
(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.
Comment 20 Leandro Graciá Gil 2012-09-17 01:05:19 PDT
Created attachment 164354 [details]
Patch
Comment 21 Leandro Graciá Gil 2012-09-17 01:39:57 PDT
Created attachment 164356 [details]
Patch
Comment 22 Leandro Graciá Gil 2012-09-17 01:40:47 PDT
(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.
Comment 23 Leandro Graciá Gil 2012-09-17 06:19:54 PDT
Created attachment 164378 [details]
Patch
Comment 24 Leandro Graciá Gil 2012-09-17 06:22:29 PDT
(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.
Comment 25 Leandro Graciá Gil 2012-09-17 06:28:18 PDT
Created attachment 164382 [details]
Patch
Comment 26 Leandro Graciá Gil 2012-09-17 06:30:02 PDT
(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.
Comment 27 Adam Barth 2012-09-17 11:27:52 PDT
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.
Comment 28 WebKit Review Bot 2012-09-17 11:35:44 PDT
Comment on attachment 164382 [details]
Patch

Clearing flags on attachment: 164382

Committed r128784: <http://trac.webkit.org/changeset/128784>
Comment 29 WebKit Review Bot 2012-09-17 11:35:49 PDT
All reviewed patches have been landed.  Closing bug.