Bug 157718 - RenderLayer::hitTestList could mutate the list of candidate layers.
Summary: RenderLayer::hitTestList could mutate the list of candidate layers.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-05-14 18:43 PDT by zalan
Modified: 2016-10-24 10:04 PDT (History)
7 users (show)

See Also:


Attachments
WIP patch (7.23 KB, patch)
2016-05-14 18:49 PDT, zalan
no flags Details | Formatted Diff | Diff
WIP patch (7.18 KB, patch)
2016-05-14 19:28 PDT, zalan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-yosemite (1.26 MB, application/zip)
2016-05-14 20:19 PDT, Build Bot
no flags Details
Patch (9.07 KB, patch)
2016-05-15 05:16 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (1.62 KB, patch)
2016-05-16 13:34 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2016-05-14 18:43:47 PDT
while looping through the list of layers.
RenderLayer::hitTestList()
...
    for (size_t i = list->size(); i > 0; --i) {
        RenderLayer* childLayer = list->at(i - 1);
        hitLayer = childLayer->hitTestLayer(rootLayer, this, request, tempResult, hitTestRect, hitTestLocation, false, transformState, zOffsetForDescendants);
    !!! list items could be invalid at this point.
   }
Comment 1 zalan 2016-05-14 18:44:12 PDT
rdar://problem/22556046
Comment 2 zalan 2016-05-14 18:49:26 PDT
Created attachment 278952 [details]
WIP patch

Not sure whether we should return nullptr or resultLayer. and whether the mutate check should happen at the end of the loop so that we could still return the current layer if it is the hittest candidate even if while hit testing it, the list got mutated.
Comment 3 WebKit Commit Bot 2016-05-14 19:12:09 PDT
Attachment 278952 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderLayer.cpp:5390:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebCore/rendering/RenderLayer.cpp:5389:  Missing space before ( in switch(  [whitespace/parens] [5]
ERROR: Source/WebCore/rendering/RenderLayer.h:823:  The parameter name "request" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/rendering/RenderLayer.h:823:  The parameter name "result" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 zalan 2016-05-14 19:28:59 PDT
Created attachment 278953 [details]
WIP patch
Comment 5 Build Bot 2016-05-14 20:19:34 PDT
Comment on attachment 278953 [details]
WIP patch

Attachment 278953 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1323073

Number of test failures exceeded the failure limit.
Comment 6 Build Bot 2016-05-14 20:19:38 PDT
Created attachment 278954 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 7 zalan 2016-05-15 05:16:56 PDT
Created attachment 278965 [details]
Patch
Comment 8 zalan 2016-05-16 13:34:35 PDT
Created attachment 279039 [details]
Patch
Comment 9 WebKit Commit Bot 2016-05-16 14:59:34 PDT
Comment on attachment 279039 [details]
Patch

Clearing flags on attachment: 279039

Committed r200971: <http://trac.webkit.org/changeset/200971>
Comment 10 WebKit Commit Bot 2016-05-16 14:59:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Darin Adler 2016-05-23 13:19:44 PDT
Comment on attachment 279039 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:1141
> +    // We should always start hittesting a clean tree.
> +    renderView->document().updateLayoutIgnorePendingStylesheets();

Seems safer to do this earlier, before calling contentRenderer(). Probably even before making the HitTestResult.

Also, “hit testing” is two words.