Bug 79136 - Hit testing may be incorrect with perspective transforms
Summary: Hit testing may be incorrect with perspective transforms
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shawn Singh
URL:
Keywords: InRadar
: 79664 (view as bug list)
Depends on: 80806
Blocks: 79664
  Show dependency treegraph
 
Reported: 2012-02-21 11:54 PST by Shawn Singh
Modified: 2012-05-29 16:44 PDT (History)
9 users (show)

See Also:


Attachments
test case that causes crash in both recent webkit and chromium builds. (3.24 KB, text/html)
2012-02-21 15:22 PST, Shawn Singh
no flags Details
Patch (13.71 KB, patch)
2012-05-03 18:25 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Fixed patch (12.56 KB, patch)
2012-05-04 12:41 PDT, Shawn Singh
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shawn Singh 2012-02-21 11:54:42 PST
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
Comment 1 Daniels 2012-02-21 14:57:50 PST
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.
Comment 2 Shawn Singh 2012-02-21 15:04:47 PST
+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.
Comment 3 Simon Fraser (smfr) 2012-02-21 15:13:59 PST
I can't see https://oreo-android.googlegoro.com/zindex-chrome-bug.html 

Please attach the testcase.
Comment 4 Shawn Singh 2012-02-21 15:22:00 PST
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&)
Comment 5 Simon Fraser (smfr) 2012-02-27 09:37:16 PST
Bug 79664 may be related.
Comment 6 Simon Fraser (smfr) 2012-02-27 09:57:07 PST
As you're probably aware, the fix for bug 62797 is related to this problem.
Comment 7 Shawn Singh 2012-05-01 17:41:15 PDT
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.
Comment 8 Simon Fraser (smfr) 2012-05-01 17:48:06 PDT
Your analysis sounds reasonable.
Comment 9 Shawn Singh 2012-05-02 10:36:48 PDT
*** Bug 79664 has been marked as a duplicate of this bug. ***
Comment 10 Shawn Singh 2012-05-03 18:25:16 PDT
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
Comment 11 WebKit Review Bot 2012-05-03 18:27:49 PDT
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.
Comment 12 Simon Fraser (smfr) 2012-05-03 18:43:26 PDT
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?
Comment 13 Simon Fraser (smfr) 2012-05-03 18:44:32 PDT
(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.
Comment 14 Shawn Singh 2012-05-03 19:30:45 PDT
(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 =)
Comment 15 Shawn Singh 2012-05-04 12:41:23 PDT
Created attachment 140300 [details]
Fixed patch

Fully tested on mac, no obvious regressions on all layout tests for both chromium and safari side.
Comment 16 WebKit Review Bot 2012-05-04 12:45:12 PDT
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.
Comment 17 Shawn Singh 2012-05-04 16:09:27 PDT
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.
Comment 18 Shawn Singh 2012-05-09 09:54:15 PDT
Still waiting for review.  Thanks!
Comment 19 Simon Fraser (smfr) 2012-05-09 10:22:46 PDT
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).
Comment 20 Shawn Singh 2012-05-09 10:57:11 PDT
Committed r116543: <http://trac.webkit.org/changeset/116543>
Comment 21 Radar WebKit Bug Importer 2012-05-24 12:14:40 PDT
<rdar://problem/11526978>
Comment 22 Shawn Singh 2012-05-29 10:25:20 PDT
(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
Comment 23 Shawn Singh 2012-05-29 10:25:52 PDT
(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
Comment 24 Dean Jackson 2012-05-29 16:44:08 PDT
(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.