Bug 79136 – Hit testing may be incorrect with perspective transforms

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

Specifically this link illustrates the problem: https://oreo-android.googlegoro.com/zindex-chrome-bug.html -- For the 3rd and 4th text containers, text cannot be selected.  This reproduces on recent builds of webkit and chromium.

My first guess is that it is related to hit testing when there are perspective transforms involved, but I have not looked in detail yet.

More details at: http://code.google.com/p/chromium/issues/detail?id=113450

[Daniels]

Here's a demo to replicate the bug:
http://savedbythegoog.appspot.com/?id=b786acdbedaced58c9f5361a4b5fefae00807b7c


(details below copied from other issue)
========================

4 elements translated negatively across the z-axis at -400px per element.  So they're located at the following depths, respectively:

0 => 0px
1 => -400px
2 => -800px
3 => -1200px

There is a content container wrapping these child elements that translates in the positive z direction at 0, 400, 800, and 1200.

Perspective value is set to 800.

When the content container is translated to a z-position greater than OR equal to the perspective value (i.e. 600px), mouse focus is lost inside the container and all child elements within.

[Shawn Singh]

+simon.fraser

Simon, I was suggested to ask you if you have a strong idea where the problem may be.  Your expertise looking at this for 2 minutes could save us hours or days of debugging =)

In particular, please see the test case on Comment #1... box C causes crashes in recent builds for both WebKit and Chromium.  Thanks in advance.

[Simon Fraser (smfr)]

I can't see https://oreo-android.googlegoro.com/zindex-chrome-bug.html 

Please attach the testcase.

[Shawn Singh]

Created attachment 128056 [details]
test case that causes crash in both recent webkit and chromium builds.

Sorry for the confusion, was referring to Daniel's first comment.   I attached it anyway.

Also, for your convenience, Here is a stack trace from chromium.

ASSERTION FAILED: !isnan(f)
../../third_party/WebKit/Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp(589) : float WebCore::clampEdgeValue(float)
1   0x2ffab92a WebCore::clampEdgeValue(float)
2   0x2ffab627 WebCore::TransformationMatrix::clampedBoundsOfProjectedQuad(WebCore::FloatQuad const&) const
3   0x3014ba5f WebCore::HitTestingTransformState::boundsOfMappedQuad() const
4   0x302baad1 WebCore::RenderLayer::hitTestLayer(WebCore::RenderLayer*, WebCore::RenderLayer*, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::IntRect const&, WebCore::IntPoint const&, bool, WebCore::HitTestingTransformState const*, double*)
5   0x302bc9fc WebCore::RenderLayer::hitTestList(WTF::Vector<WebCore::RenderLayer*, 0ul>*, WebCore::RenderLayer*, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::IntRect const&, WebCore::IntPoint const&, WebCore::HitTestingTransformState const*, double*, double*, WebCore::HitTestingTransformState const*, bool)
6   0x302bb171 WebCore::RenderLayer::hitTestLayer(WebCore::RenderLayer*, WebCore::RenderLayer*, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::IntRect const&, WebCore::IntPoint const&, bool, WebCore::HitTestingTransformState const*, double*)
7   0x302bc9fc WebCore::RenderLayer::hitTestList(WTF::Vector<WebCore::RenderLayer*, 0ul>*, WebCore::RenderLayer*, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::IntRect const&, WebCore::IntPoint const&, WebCore::HitTestingTransformState const*, double*, double*, WebCore::HitTestingTransformState const*, bool)
8   0x302bb171 WebCore::RenderLayer::hitTestLayer(WebCore::RenderLayer*, WebCore::RenderLayer*, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::IntRect const&, WebCore::IntPoint const&, bool, WebCore::HitTestingTransformState const*, double*)
9   0x302bc9fc WebCore::RenderLayer::hitTestList(WTF::Vector<WebCore::RenderLayer*, 0ul>*, WebCore::RenderLayer*, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::IntRect const&, WebCore::IntPoint const&, WebCore::HitTestingTransformState const*, double*, double*, WebCore::HitTestingTransformState const*, bool)
10  0x302bb171 WebCore::RenderLayer::hitTestLayer(WebCore::RenderLayer*, WebCore::RenderLayer*, WebCore::HitTestRequest const&, WebCore::HitTestResult&, WebCore::IntRect const&, WebCore::IntPoint const&, bool, WebCore::HitTestingTransformState const*, double*)
11  0x302ba5d1 WebCore::RenderLayer::hitTest(WebCore::HitTestRequest const&, WebCore::HitTestResult&)
12  0x33a18c3b WebCore::Document::prepareMouseEvent(WebCore::HitTestRequest const&, WebCore::IntPoint const&, WebCore::PlatformMouseEvent const&)
13  0x2f79d767 WebCore::EventHandler::prepareMouseEvent(WebCore::HitTestRequest const&, WebCore::PlatformMouseEvent const&)
14  0x2f79dd69 WebCore::EventHandler::handleMouseMoveEvent(WebCore::PlatformMouseEvent const&, WebCore::HitTestResult*, bool)
15  0x2f79d860 WebCore::EventHandler::mouseMoved(WebCore::PlatformMouseEvent const&)
16  0x307703ad WebKit::WebViewImpl::mouseMove(WebKit::WebMouseEvent const&)

[Simon Fraser (smfr)]

Bug 79664 may be related.

[Simon Fraser (smfr)]

As you're probably aware, the fix for bug 62797 is related to this problem.

[Shawn Singh]

Update:

The problem is actually a tangle of 3 different bugs in the projectPoint / projectQuad code.

(1) m33() divide-by-zero was not being handled, causing Infs and NaNs.   This happens when the projection plane is perpendicular to the ray we are trying to project, and I think in this case the projection is simply undefined.

(2) The clamping to int max seems to be causing overflow.  If I lowered the values down to 100 million, the same bugs do not appear (after including fixes for (1) and (3), too)

(3a) projectQuad() is ignoring when projectPoint says that things are clamped.  if every point is clamped, we should be returning an empty quad here.

(3b) If some points are clamped, the problem is much, much harder.  Technically the correct way to solve this is to do clipping in homogeneous coordinates, before divide-by-w.  However, this will cause general polygons (triangles, pentagons, polygons of up to 8 vertices), and that change would have to propagate to a substantial chunk of hit testing code.   For now, I can opt for a "least evil solution" that, as best as I can test, has not yet broken any layout tests or websites that I tried.

I'll work on creating layout tests to cover the fix and then submit a patch.

[Simon Fraser (smfr)]

Your analysis sounds reasonable.

[Shawn Singh]

*** Bug 79664 has been marked as a duplicate of this bug. ***

[Shawn Singh]

Created attachment 140140 [details]
Patch

There is one style issue about checking whether m33() == 0.  If its OK, I would prefer to keep it the way it is for readability, most of the rest of TransformationMatrix.cpp already does that.  Also, I tested all layout tests on chromium, no obvious regressions.  I tried to test Safari side layout tests too, but so many tests are currently failing that its meaningless.  However, I still feel decently confident that this patch works as intended

[WebKit Review Bot]

Attachment 140140 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/tran..." exit_code: 1
Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:555:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 140140 [details]
Patch

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

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:625
> +    if (somethingWasClipped)
> +        return FloatQuad();

Won't this break the case that the clamping was added to fix in the first place?

[Simon Fraser (smfr)]

(In reply to comment #10)
> I tried to test Safari side layout tests too, but so many tests are currently failing that its meaningless.  However, I still feel decently confident that this patch works as intended

Which are failing? They are passing on the bots.

[Shawn Singh]

(In reply to comment #12)
> (From update of attachment 140140 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140140&action=review
> 
> > Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:625
> > +    if (somethingWasClipped)
> > +        return FloatQuad();
> 
> Won't this break the case that the clamping was added to fix in the first place?

I tested those specifically, and they still worked.  I'll double-check again tomorrow.


(In reply to comment #13)
> (In reply to comment #10)
> > I tried to test Safari side layout tests too, but so many tests are currently failing that its meaningless.  However, I still feel decently confident that this patch works as intended
> 
> Which are failing? They are passing on the bots.

I checked again and it looks like the bots are doing better now.  Earlier when I was running them, it was giving up after 500 failures, only having tested 3000 tests.   I'll run these again tomorrow, too =)

[Shawn Singh]

Created attachment 140300 [details]
Fixed patch

Fully tested on mac, no obvious regressions on all layout tests for both chromium and safari side.

[WebKit Review Bot]

Attachment 140300 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/tran..." exit_code: 1
Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:555:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Shawn Singh]

Simon, could you please review this, when you have a chance?  Thanks very much in advance.

As before, I'm requesting if we can overrule the one style issue about checking m33() == 0, since the rest of TransformationMatrix.cpp does that anyway, and it is more clear.

[Shawn Singh]

Still waiting for review.  Thanks!

[Simon Fraser (smfr)]

Comment on attachment 140300 [details]
Fixed patch

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

> LayoutTests/transforms/3d/hit-testing/coplanar-with-camera.html:15
> +        -webkit-perspective: 800;

Needs units (px).

> LayoutTests/transforms/3d/hit-testing/perspective-clipped.html:12
> +        -webkit-perspective: 1000;

You should use units on perspective (1000px).

[Shawn Singh]

Committed r116543: <http://trac.webkit.org/changeset/116543>

[Radar WebKit Bug Importer]

<rdar://problem/11526978>

[Shawn Singh]

(In reply to comment #21)
> <rdar://problem/11526978>

Perhaps by chance, comments #34 and #35 are related to the rdar bug?

http://code.google.com/p/chromium/issues/detail?id=113450

[Shawn Singh]

(In reply to comment #22)
> (In reply to comment #21)
> > <rdar://problem/11526978>
> 
> Perhaps by chance, comments #34 and #35 are related to the rdar bug?
> 
> http://code.google.com/p/chromium/issues/detail?id=113450

To clarify, I mean comments #34 and #35 in the chromium bug that I linked - http://code.google.com/p/chromium/issues/detail?id=113450

[Dean Jackson]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > <rdar://problem/11526978>
> > 
> > Perhaps by chance, comments #34 and #35 are related to the rdar bug?

The rdar bug is just a duplicate of this bug that we use for internal tracking.