Bug 97688 - [Chromium] Fix find-in-page corner case for detached frames
Summary: [Chromium] Fix find-in-page corner case for detached frames
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:
Blocks:
 
Reported: 2012-09-26 08:45 PDT by Leandro Graciá Gil
Modified: 2012-09-26 10:27 PDT (History)
3 users (show)

See Also:

Attachments
Patch (1.56 KB, patch)
2012-09-26 08:50 PDT, Leandro Graciá Gil 		no flags Details | Formatted Diff | Diff
Patch (1.57 KB, patch)
2012-09-26 09:17 PDT, Leandro Graciá Gil 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Leandro Graciá Gil 2012-09-26 08:45:57 PDT 
The find-in-page code refactoring introduced by 96402 to prevent cases where the final update was not sent made the request reset happen before the shouldScopeMatches check. This check verifies if the view is available and prevented any further processing on detached frames. With the newly introduced changes, one of the operations performed during reset tries to access the page, leading to crashes on detached frame cases. This should be properly checked before trying to access the page.
Comment 1 Leandro Graciá Gil 2012-09-26 08:50:30 PDT 
Created attachment 165813 [details]
Patch
Comment 2 WebKit Review Bot 2012-09-26 09:11:00 PDT 
Comment on attachment 165813 [details]
Patch

Attachment 165813 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14030525
Comment 3 Adam Barth 2012-09-26 09:14:45 PDT 
Comment on attachment 165813 [details]
Patch

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

We really need better testing for this feature.  I don't want to hold up this patch over lack of testing, but that's the reason we're having these sorts of troubles.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1734
> -        if (frame() && frame()->editor()->markedTextMatchesAreHighlighted())
> +        if (frame() && frame->page() && frame()->editor()->markedTextMatchesAreHighlighted())

frame -> frame()
Comment 4 Leandro Graciá Gil 2012-09-26 09:17:54 PDT 
Created attachment 165822 [details]
Patch
Comment 5 Leandro Graciá Gil 2012-09-26 09:18:05 PDT 
Comment on attachment 165813 [details]
Patch

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

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1734
>> +        if (frame() && frame->page() && frame()->editor()->markedTextMatchesAreHighlighted())
> 
> frame -> frame()

Fixed. Sorry for the nit.
Comment 6 Leandro Graciá Gil 2012-09-26 09:19:01 PDT 
(In reply to comment #3)
> (From update of attachment 165813 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165813&action=review
> 
> We really need better testing for this feature.  I don't want to hold up this patch over lack of testing, but that's the reason we're having these sorts of troubles.

Completely agree. It proved to be trickier than it seems and with corner cases that need proper coverage.
Comment 7 Adam Barth 2012-09-26 10:04:25 PDT 
Comment on attachment 165822 [details]
Patch

ok...
Comment 8 WebKit Review Bot 2012-09-26 10:27:07 PDT 
Comment on attachment 165822 [details]
Patch

Clearing flags on attachment: 165822

Committed r129666: <http://trac.webkit.org/changeset/129666>
Comment 9 WebKit Review Bot 2012-09-26 10:27:11 PDT 
All reviewed patches have been landed.  Closing bug.