97688 2012-09-26 08:45:57 -0700 [Chromium] Fix find-in-page corner case for detached frames 2012-09-26 10:27:11 -0700 1 1 1 Unclassified WebKit WebKit Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 leandrogracia leandrogracia abarth dglazkov webkit.review.bot oldest_to_newest 728601 0 leandrogracia 2012-09-26 08:45:57 -0700 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. 728610 1 165813 leandrogracia 2012-09-26 08:50:30 -0700 Created attachment 165813 Patch 728638 2 165813 webkit.review.bot 2012-09-26 09:11:00 -0700 Comment on attachment 165813 Patch Attachment 165813 did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14030525 728647 3 165813 abarth 2012-09-26 09:14:45 -0700 Comment on attachment 165813 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() 728652 4 165822 leandrogracia 2012-09-26 09:17:54 -0700 Created attachment 165822 Patch 728653 5 165813 leandrogracia 2012-09-26 09:18:05 -0700 Comment on attachment 165813 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. 728654 6 leandrogracia 2012-09-26 09:19:01 -0700 (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. 728694 7 165822 abarth 2012-09-26 10:04:25 -0700 Comment on attachment 165822 Patch ok... 728709 8 165822 webkit.review.bot 2012-09-26 10:27:07 -0700 Comment on attachment 165822 Patch Clearing flags on attachment: 165822 Committed r129666: <http://trac.webkit.org/changeset/129666> 728710 9 webkit.review.bot 2012-09-26 10:27:11 -0700 All reviewed patches have been landed. Closing bug. 165813 2012-09-26 08:50:30 -0700 2012-09-26 09:18:05 -0700 Patch bug-97688-20120926164948.patch text/plain 1602 leandrogracia U3VidmVyc2lvbiBSZXZpc2lvbjogMTI5NjMxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2No cm9taXVtL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCmluZGV4 IGNkMGEwYzg2MzE3MGQ3MmFmNTE3NDE1Y2VhMzhhMWM4ZDM5NDFiZjIuLjk3NGRkNWQzOTdkYjBm ZGM5MWJlN2FjODE5NWE3ZGE3NTFlZWMyMzUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvY2hy b21pdW0vQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCkBA IC0xLDMgKzEsMTYgQEAKKzIwMTItMDktMjYgIExlYW5kcm8gR3JhY2lhIEdpbCAgPGxlYW5kcm9n cmFjaWFAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFtDaHJvbWl1bV0gRml4IGZpbmQtaW4tcGFn ZSBjb3JuZXIgY2FzZSBmb3IgZGV0YWNoZWQgZnJhbWVzCisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD05NzY4OAorCisgICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgorCisgICAgICAgIEFkZHMgYSBjaGVjayB0byBwcmV2ZW50IGNyYXNoZXMg d2hlbiBydW5uaW5nIGZpbmQtaW4tcGFnZSByZXF1ZXN0cworICAgICAgICBvbiBkZXRhY2hlZCBm cmFtZXMuCisKKyAgICAgICAgKiBzcmMvV2ViRnJhbWVJbXBsLmNwcDoKKyAgICAgICAgKFdlYktp dDo6V2ViRnJhbWVJbXBsOjpzY29wZVN0cmluZ01hdGNoZXMpOgorCiAyMDEyLTA5LTI1ICBBbmRy ZXkgS29zeWFrb3YgIDxjYXNlcUBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgV2ViIEluc3BlY3Rv cjogW2Nocm9taXVtXSBhZGQgYSB0ZXN0IGZvciBpbnNwZWN0b3Igb3ZlcmxheSB1cGRhdGUgZnJl cXVlbmN5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9XZWJGcmFtZUlt cGwuY3BwIGIvU291cmNlL1dlYktpdC9jaHJvbWl1bS9zcmMvV2ViRnJhbWVJbXBsLmNwcAppbmRl eCA2ZDY4YTZmMjNlMzdkNTUyMGUwZTYzNWU1YzBlZmZiMmM3ZmNhYTZlLi4yZGU5MGY5Y2E5MjU3 NzM3NmU3NTZiNDEwMTZkMTljMWQ2Mjc4ZmVlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0L2No cm9taXVtL3NyYy9XZWJGcmFtZUltcGwuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0v c3JjL1dlYkZyYW1lSW1wbC5jcHAKQEAgLTE3MzEsNyArMTczMSw3IEBAIHZvaWQgV2ViRnJhbWVJ bXBsOjpzY29wZVN0cmluZ01hdGNoZXMoaW50IGlkZW50aWZpZXIsCiAgICAgICAgIG1fc2NvcGlu Z0NvbXBsZXRlID0gZmFsc2U7CiAKICAgICAgICAgLy8gQ2xlYXIgaGlnaGxpZ2h0aW5nIGZvciB0 aGlzIGZyYW1lLgotICAgICAgICBpZiAoZnJhbWUoKSAmJiBmcmFtZSgpLT5lZGl0b3IoKS0+bWFy a2VkVGV4dE1hdGNoZXNBcmVIaWdobGlnaHRlZCgpKQorICAgICAgICBpZiAoZnJhbWUoKSAmJiBm cmFtZS0+cGFnZSgpICYmIGZyYW1lKCktPmVkaXRvcigpLT5tYXJrZWRUZXh0TWF0Y2hlc0FyZUhp Z2hsaWdodGVkKCkpCiAgICAgICAgICAgICBmcmFtZSgpLT5wYWdlKCktPnVubWFya0FsbFRleHRN YXRjaGVzKCk7CiAKICAgICAgICAgLy8gQ2xlYXIgdGhlIHRpY2ttYXJrcyBhbmQgcmVzdWx0cyBj YWNoZS4K 165822 2012-09-26 09:17:54 -0700 2012-09-26 10:27:07 -0700 Patch bug-97688-20120926171712.patch text/plain 1604 leandrogracia U3VidmVyc2lvbiBSZXZpc2lvbjogMTI5NjMxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2No cm9taXVtL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCmluZGV4 IGNkMGEwYzg2MzE3MGQ3MmFmNTE3NDE1Y2VhMzhhMWM4ZDM5NDFiZjIuLjk3NGRkNWQzOTdkYjBm ZGM5MWJlN2FjODE5NWE3ZGE3NTFlZWMyMzUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvY2hy b21pdW0vQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCkBA IC0xLDMgKzEsMTYgQEAKKzIwMTItMDktMjYgIExlYW5kcm8gR3JhY2lhIEdpbCAgPGxlYW5kcm9n cmFjaWFAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFtDaHJvbWl1bV0gRml4IGZpbmQtaW4tcGFn ZSBjb3JuZXIgY2FzZSBmb3IgZGV0YWNoZWQgZnJhbWVzCisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD05NzY4OAorCisgICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgorCisgICAgICAgIEFkZHMgYSBjaGVjayB0byBwcmV2ZW50IGNyYXNoZXMg d2hlbiBydW5uaW5nIGZpbmQtaW4tcGFnZSByZXF1ZXN0cworICAgICAgICBvbiBkZXRhY2hlZCBm cmFtZXMuCisKKyAgICAgICAgKiBzcmMvV2ViRnJhbWVJbXBsLmNwcDoKKyAgICAgICAgKFdlYktp dDo6V2ViRnJhbWVJbXBsOjpzY29wZVN0cmluZ01hdGNoZXMpOgorCiAyMDEyLTA5LTI1ICBBbmRy ZXkgS29zeWFrb3YgIDxjYXNlcUBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgV2ViIEluc3BlY3Rv cjogW2Nocm9taXVtXSBhZGQgYSB0ZXN0IGZvciBpbnNwZWN0b3Igb3ZlcmxheSB1cGRhdGUgZnJl cXVlbmN5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9XZWJGcmFtZUlt cGwuY3BwIGIvU291cmNlL1dlYktpdC9jaHJvbWl1bS9zcmMvV2ViRnJhbWVJbXBsLmNwcAppbmRl eCA2ZDY4YTZmMjNlMzdkNTUyMGUwZTYzNWU1YzBlZmZiMmM3ZmNhYTZlLi40NGY1ZTE3M2QxODQw NmMwNjhkNzVhYTMzZTNmMGZmOWY4NWQyMTVmIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0L2No cm9taXVtL3NyYy9XZWJGcmFtZUltcGwuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0v c3JjL1dlYkZyYW1lSW1wbC5jcHAKQEAgLTE3MzEsNyArMTczMSw3IEBAIHZvaWQgV2ViRnJhbWVJ bXBsOjpzY29wZVN0cmluZ01hdGNoZXMoaW50IGlkZW50aWZpZXIsCiAgICAgICAgIG1fc2NvcGlu Z0NvbXBsZXRlID0gZmFsc2U7CiAKICAgICAgICAgLy8gQ2xlYXIgaGlnaGxpZ2h0aW5nIGZvciB0 aGlzIGZyYW1lLgotICAgICAgICBpZiAoZnJhbWUoKSAmJiBmcmFtZSgpLT5lZGl0b3IoKS0+bWFy a2VkVGV4dE1hdGNoZXNBcmVIaWdobGlnaHRlZCgpKQorICAgICAgICBpZiAoZnJhbWUoKSAmJiBm cmFtZSgpLT5wYWdlKCkgJiYgZnJhbWUoKS0+ZWRpdG9yKCktPm1hcmtlZFRleHRNYXRjaGVzQXJl SGlnaGxpZ2h0ZWQoKSkKICAgICAgICAgICAgIGZyYW1lKCktPnBhZ2UoKS0+dW5tYXJrQWxsVGV4 dE1hdGNoZXMoKTsKIAogICAgICAgICAvLyBDbGVhciB0aGUgdGlja21hcmtzIGFuZCByZXN1bHRz IGNhY2hlLgo=