Assertion failure: non-invertable matrix inverted in HitTestingTreeNode::Untransform

RESOLVED DUPLICATE of bug 1352564

Status

()

Core
Panning and Zooming
P3
normal
RESOLVED DUPLICATE of bug 1352564
a year ago
a year ago

People

(Reporter: miko, Assigned: Dean Zhu, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [gfx-noted] [lang=c++] [good first bug])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
Created attachment 8847114 [details]
untransform_crash.html

I was trying to trigger a similar assertion within the layers code and found this instead. The current m-c debug build crashes with assertion in HitTestingTreeNode::Untransform(), when attempting to invert a non-invertible matrix.

To reproduce:
1. Open the attached testcase in a debug build
2. Scroll the page for a couple of seconds

Reproduced on OS X and Windows 10.

Stack trace:
Assertion failure: inverted (Attempted to get the inverse of a non-invertible matrix), at include/mozilla/gfx/Matrix.h:1235
#01: mozilla::EnableIf<(IsSame<decltype(fp0(fp)), mozilla::layers::TraversalFlag>::value) && (IsSame<decltype(fp1(fp)), mozilla::layers::TraversalFlag>::value), bool>::Type mozilla::layers::ForEachNode<mozilla::layers::ReverseIterator, mozilla::layers::HitTest
#05: mozilla::layers::APZCTreeManager::GetAPZCAtPoint(mozilla::layers::HitTestingTreeNode*, mozilla::gfx::PointTyped<mozilla::ParentLayerPixel, float> const&, mozilla::layers::HitTestResult*, bool*)
#06: mozilla::layers::APZCTreeManager::GetTargetAPZC(mozilla::gfx::PointTyped<mozilla::ScreenPixel, float> const&, mozilla::layers::HitTestResult*, bool*)
#07: mozilla::layers::APZCTreeManager::ReceiveInputEvent(mozilla::InputData&, mozilla::layers::ScrollableLayerGuid*, unsigned long long*)
#08: nsChildView::DispatchAPZWheelInputEvent(mozilla::InputData&, bool)
We should probably add a InverseIfPossible or some such function to Matrix.h that returns a Maybe<Matrix>. Then at the call site we can do a better job of using or skipping it. Right now the only two options are calling Invert() which returns a bool and is not type-safe, or Inverse() which asserts in the failure case.
Priority: -- → P3
Whiteboard: [gfx-noted]
Actually that's kind of overkill for this case. We can just use Invert().
Makes a good first bug.
Mentor: botond
Whiteboard: [gfx-noted] → [gfx-noted] [lang=c++] [good first bug]

Comment 4

a year ago
How to start with this bug
(In reply to ppaswanth3 from comment #4)
> How to start with this bug

Hi! Thanks for your interest.

The first step is to get a working build of Firefox. Please follow the instructions here [1], and let me know if you run into any issues. Once that's done I'll give some suggestions for starting with this bug specifically.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
(Assignee)

Comment 6

a year ago
Hi,
I just built firefox on my linux machine and I wondered if could start working on this bug.
(In reply to Dean Zhu from comment #6)
> Hi,
> I just built firefox on my linux machine and I wondered if could start
> working on this bug.

Hey Dean, ppaswanth3 has already expressed interest in working on this bug in comment 4, I'd like to give them a chance to do it.

You can find other bugs available to work on here: https://www.joshmatthews.net/bugsahoy/
(Assignee)

Comment 8

a year ago
(In reply to Botond Ballo [:botond] from comment #7)
> (In reply to Dean Zhu from comment #6)
> > Hi,
> > I just built firefox on my linux machine and I wondered if could start
> > working on this bug.
> 
> Hey Dean, ppaswanth3 has already expressed interest in working on this bug
> in comment 4, I'd like to give them a chance to do it.
> 
> You can find other bugs available to work on here:
> https://www.joshmatthews.net/bugsahoy/

Hi,
Sorry about that, I was a little too excited and that somehow flew over my head. In anycase, even without submitting a fix, I'd still like to fidget a little bit with this bug to get a grasp of the workflow. Would it be ok if I sent you a mail in the event there is something I don't understand?
(In reply to Dean Zhu from comment #8)
> Would it be ok if I
> sent you a mail in the event there is something I don't understand?

Sure thing! You can also find me on IRC [1] in the #apz channel (if I don't respond then please do send email).

[1] https://wiki.mozilla.org/IRC
Dean, since we haven't heard from ppaswanth3 yet - if you're still interested in working on this, please go ahead! Let me know if you'd like some pointers on how to start.
(Assignee)

Comment 11

a year ago
Hi, sure I'm up for it.
I had two questions/problem regarding this bug.
1) I couldn't reproduce the bug in my linux setup, or I didn't reproduce it correctly.
2) If we see that a matrix is irreversible what should we do?

Comment 12

a year ago
(In reply to Botond Ballo [:botond] from comment #10)
> Dean, since we haven't heard from ppaswanth3 yet - if you're still
> interested in working on this, please go ahead! Let me know if you'd like
> some pointers on how to start.

Sorry for the delay 
I was unable to access internet these days I will proceed today today itself and let you know the progress
(In reply to ppaswanth3 from comment #12)
> Sorry for the delay 
> I was unable to access internet these days I will proceed today today itself
> and let you know the progress

No worries.

In the meantime, I'll answer Dean's questions anyways, as they will probably help you as well:

> 1) I couldn't reproduce the bug in my linux setup, or I didn't reproduce it
> correctly.

Assertions written using MOZ_ASSERT (like the one at issue in this bug [1]) are only checked in debug builds. The instructions linked to in comment 5 create a non-debug build by default.

You can create a debug build by adding the following to your .mozconfig file:

  ac_add_options --enable-debug

and rebuilding with |mach build|.

Alternatively, if you already have a non-debug build and don't want to do a full rebuild, you can temporarily change the MOZ_ASSERT to a MOZ_RELEASE_ASSERT to get it to be checked in a non-debug build.

> 2) If we see that a matrix is irreversible what should we do?

Matrx4x4Typed::Inverse() is intended to require as a precondition that the matrix be invertible, so asserting in its implementation is appropriate. (We could perhaps add a comment before the declaration of Inverse(), making this precondition explicit.)

For cases where a matrix may or may not be invertible, Matrix4x4Typed provides an Invert() method which modifies the matrix in-place, and returns a boolean to indicate whether the matrix could be inverted.

So, in this case, it would be appropriate to change the call site [2] to use Invert() instead of Inverse().

The calling function (HitTestingTreeNode::Untransform) already returns a Maybe to indicate possible failure, so in the case Invert() fails, it can return an empty Maybe (see the documentation of Maybe [3], and an example of its use [4]).

[1] http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/gfx/2d/Matrix.h#1235
[2] http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/gfx/layers/apz/src/HitTestingTreeNode.cpp#258
[3] http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/mfbt/Maybe.h#28
[4] http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/layout/base/UnitTransforms.h#226
Assignee: nobody → deanzhu2
(Assignee)

Comment 14

a year ago
I have compiled my patch and it seems to be ok, what should I do?
I have tried using the mq extention, but I couldn't manage putting my username on it, with
[ui]
username = ...
Both in .hg/hgrc and in mercurial.ini
If you have a patch file that contains your changes, and the only thing missing is your name, just go ahead and attach it. I can fill in the name before committing.

For future reference, the Mercurial worflow I recommend is this:

  // Make changes in your working directory
  hg commit                  // commit changes
  hg export . > patch.diff   // export commit as patch file

No mq required.

(Alternatively, you could also try using MozReview [1]).

[1] http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html
(Assignee)

Comment 16

a year ago
Hey, I just rechecked everything is in place and I have seen a bug in the code.
So Invert() checks if the matrix is invertible or not, and if it returns true it means the matrix has been inverted.
Then when I call Untransformby I should use transform as an argument and not transform.invert().
Now recompiling this it throws out an error, which if I understand correctly Untransformby doesn't allow LayerToParentLayerMatrix4x4 as an argument.
How should I proceed?

error
> src/mozilla-central/gfx/layers/apz/src/HitTestingTreeNode.cpp:260:43: error: no matching function for call to ‘UntransformBy(mozilla::LayerToParentLayerMatrix4x4&, const ParentLayerPoint&)’ 0:05.62      return UntransformBy(transform, aPoint);
(In reply to Dean Zhu from comment #16)
> Now recompiling this it throws out an error, which if I understand correctly
> Untransformby doesn't allow LayerToParentLayerMatrix4x4 as an argument.
> How should I proceed?

You're running into our strongly-typed coordinate space system.

Every coordinate space in our code has its own type, and matrices, which convert quantities from one coordinate space to another, are annotated with two types: the source coordinate system, and the target coordinate system.

Inverting a matrix swaps its source and target coordinates, since the inverted matrix converts quantities "backwards" from the target space to the source space.

In this case, we are inverting a LayerToParentLayerMatrix4x4, which should give us a ParentLayerToLayerMatrix4x4 ("Layer" and "ParentLayer" are two of the coordinate spaces we use). The Inverse() function plays nicely with this system, returning a matrix of the correct type, but Invert() does not, because it modifies the matrix in-place, and you can't change the type of an existing object.

For now, we can work around this by using ToUnknownMatrix() and FromUnknownMatrix() to cast the matrix to the correct type, the way Inverse() does [1], and then call Invert().

(As a nicer solution for the longer-term, we can follow Kats' suggestion in comment 1 and introduce a version of Inverse() that returns a Maybe<> wrapping a matrix of a correct type. This can be left for a follow-up bug.)

[1] http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/gfx/2d/Matrix.h#1233
(Assignee)

Comment 18

a year ago
Is it a basis matrix? or am I misrepresenting what you said?
I have been trying to cast but I haven't been successful at it. Can it be that I need to add an include or it is casted differently than those of the stl library(casting an int a by doing (double) a )
And in the case of needing an include, wouldn't fixing invert matrix be a better idea? Or would it be too much of a hassle to change the pre and poscondition of that method?
(In reply to Dean Zhu from comment #18)
> I have been trying to cast but I haven't been successful at it. Can it be
> that I need to add an include or it is casted differently than those of the
> stl library(casting an int a by doing (double) a )

Sorry for the confusion. By "cast" I just meant "obtain an object of a different type storing the same value", I didn't mean a C++ cast like "(double) 5".

The way to do it for matrices, is using the functions ToUnknownMatrix() and FromUnknownMatrix(), as in the code linked in comment 17. In this case it would be like:

ParentLayerToLayerMatrix4x4 casted = ParentLayerToLayerMatrix4x4::FromUnknownMatrix(transform.ToUnknownMatrix());

> And in the case of needing an include, wouldn't fixing invert matrix be a
> better idea? Or would it be too much of a hassle to change the pre and
> poscondition of that method?

It's not clear to me what you're suggesting, but I'm happy to consider an alternative approach if you describe one in more detail.
(Assignee)

Comment 20

a year ago
Oh, I thought the error which came was from using a casting function from a source file not included in the headers, not that I called it incorrectly.
So I think now It's finnaly correct, how should I upload the patch? To provide a brief summary I simply added this code:
Quite straight forward.

>  if(transform.Invert()){
>    ParentLayerToLayerMatrix4x4 casted = ParentLayerToLayerMatrix4x4::FromUnknownMatrix(transform.ToUnknownMatrix()); 
>    return UntransformBy(casted, aPoint);
>  }
>  return Nothing();
(In reply to Dean Zhu from comment #20)
> So I think now It's finnaly correct, how should I upload the patch?

The 2 options are:

  1) Attach the patch file, and flag me for review.
  2) Use MozReview (instructions linked from comment 15).
(Assignee)

Comment 22

a year ago
Created attachment 8852243 [details] [diff] [review]
Bug1347157.patch
Attachment #8852243 - Flags: review?(botond)
Comment on attachment 8852243 [details] [diff] [review]
Bug1347157.patch

Review of attachment 8852243 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good and works well!

I have just a couple of minor style nits.

Also, please include a description of what the patch does in the commit message. Something along the lines of "handle non-invertible transforms in HitTestingTreeNode::Untransform()".

::: gfx/layers/apz/src/HitTestingTreeNode.cpp
@@ +255,5 @@
>          mApzc
>        ? mApzc->GetCurrentAsyncTransformWithOverscroll(AsyncPanZoomController::NORMAL)
>        : AsyncTransformComponentMatrix());
> +
> +  if(transform.Invert()){

As per the style guide [1], please add a space before the '{'.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

@@ +256,5 @@
>        ? mApzc->GetCurrentAsyncTransformWithOverscroll(AsyncPanZoomController::NORMAL)
>        : AsyncTransformComponentMatrix());
> +
> +  if(transform.Invert()){
> +    ParentLayerToLayerMatrix4x4 casted = ParentLayerToLayerMatrix4x4::FromUnknownMatrix(transform.ToUnknownMatrix());

Break this line before the '=' to stay under the line length limit. (The continuation line should be indented by 2 levels (= 4 spaces)).
Attachment #8852243 - Flags: review?(botond) → feedback+
(And apologies for the delay in getting to the review.)
(Assignee)

Comment 25

a year ago
I'll try and fix this when I get home, is the line length limit 80 chars?
(In reply to Dean Zhu from comment #25)
> I'll try and fix this when I get home, is the line length limit 80 chars?

Yeah. Going a few characters over is fine, but the line as currently in the patch is too long.
(Assignee)

Comment 27

a year ago
Created attachment 8853529 [details] [diff] [review]
Bug1347157.patch
Attachment #8852243 - Attachment is obsolete: true
Attachment #8853529 - Flags: review?(botond)
Comment on attachment 8853529 [details] [diff] [review]
Bug1347157.patch

Review of attachment 8853529 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: gfx/layers/apz/src/HitTestingTreeNode.cpp
@@ +255,5 @@
>          mApzc
>        ? mApzc->GetCurrentAsyncTransformWithOverscroll(AsyncPanZoomController::NORMAL)
>        : AsyncTransformComponentMatrix());
> +
> +  if(transform.Invert()) {

(I didn't notice before, but there should be a space after the 'if' as well. I'll just add that before landing, no need for you to upload another version of the patch.)
Attachment #8853529 - Flags: review?(botond) → review+
See Also: → bug 1352564
(In reply to Botond Ballo [:botond] from comment #17)
> (As a nicer solution for the longer-term, we can follow Kats' suggestion in
> comment 1 and introduce a version of Inverse() that returns a Maybe<>
> wrapping a matrix of a correct type. This can be left for a follow-up bug.)

I filed bug 1352564 for this. Dean, let me know if you're interested in working on it.
(Assignee)

Comment 31

a year ago
Yeah sure, I'm up for it. what's the try server is it like a testing branch/ is there docmentation?
(In reply to Dean Zhu from comment #31)
> Yeah sure, I'm up for it.

Ok, cool! Let me know (in bug 1352564) if you have any questions about how to get started.

> what's the try server is it like a testing branch/ is there docmentation?

It is a mercurial server that you can push commits to [1], which will trigger a run of all our automated tests, whose results you can see on Treeherder (linked to in comment 29). It is recommended to push changes to the try server before committing them.

Pushing changes to the try server requires Level 1 commit access [2], which you can probably apply for after submitting a couple more patches.

[1] https://hg.mozilla.org/try/
[2] https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/
The patch in bug 1352564 will fix the issue, so there's no need to commit the patch posted in this bug.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1352564
You need to log in before you can comment on or make changes to this bug.