425662 - Scaled SVG content can get the wrong metrics and poison the cache

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Duncan Loveday]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031806 Minefield/3.0b5pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031806 Minefield/3.0b5pre

I'm seeing an intermittent problem where firefox gets completely confused about what is under the mouse - onclick events not firing or being routed to the wrong elements, mouseover/mouseout events not firing, hyperlinks not working or wrong hyperlinks activating. As I move the mouse pointer around the screen the status bar changes to show the hyperlinks which will activate on a click but they're not the ones under the mouse. As if the image it thinks is there is at a different offset and/or scale.

When in this state, if I zoom in or out then everything starts working. But zoom back to the original scale and the problem returns.

Regrettably I can NOT reproduce in a small test case - only my real app which is far too complex to server as a test case. And even there that application the problem is intermittent. I think there might be some sort of race condition with page load events vs zoom events or something like that. My app zooms the image as part of page load. I'll keep trying but no luck so far.

The only thing I have for definite is the regression range which is between the 18/3/2008 06 nightly and the 19/3/2008 05 nightly.

Reproducible: Sometimes

Steps to Reproduce:
No test case yet

Comment 1

[Robert Longson [:longsonr]]

The date range corresponds to the check in for bug 392233

Comment 2

[Duncan Loveday]

Would there be any point in me providing a hugely complex test case so that at least you guys could see the effect ? I'm getting nowhere with minimal test cases.

Otherwise, if bug 392233 is a suspect is there a way I could get a build with just that change backed out to see if that is the culprit ?

Comment 3

[Robert Longson [:longsonr]]

I think your best bet is to use a prior to that date nightly.

Comment 4

[Robert Longson [:longsonr]]

It's possible that the patch in bug 423998 will fix things for you.

Comment 5

[Duncan Loveday]

(In reply to comment #3)
> I think your best bet is to use a prior to that date nightly.
> 
Robert - I was looking for a way of confirming specifically whether that one change you mention is what's causing the issue. I know already that the previous day's build does not have the issue but I assume there will be loads of other differences in the same time range.

Looking at the bugs you've mentioned it might be that I need to include text in order to reproduce the issue - I'll see if I can confirm that removing text from my real app makes the problem go away. If so, I'll have another crack at a test case with some text in it. As always, thanks for the info.

Comment 6

[Duncan Loveday]

Attachment: SVG test case

Comment 7

[Duncan Loveday]

Attachment: HTML test case

I have a test case for at least part of this issue now. To reproduce

1) Open the HTML attachment
2 [review]) Click on any of the rectangles or text. Notice the alert reports "y=4" for all of them when in fact they should all have different y values.
3) Clock to the right of the rectangle area - notice that a hyperlink is activated when none is present under the mouse
4) Also notice that the mouse pointer and status bar show values that are not the hyperlinks that are under the mouse.

Comment 8

[Duncan Loveday]

Guys - the test HTML I've attached renders at a completely different scale when it's live in the bug as compared to when opened as a local file. As a result, opening the live file doesn't reproduce the issue. To reproduce, save the HTML and SVG examples, change the link in the HTML to point to the locally-stored SVG and File->open to open the HTML in Firefox.

It must be my fault but I just can't see why the scaling is different when opened as a local file vs over HTTP.

Comment 9

[Duncan Loveday]

Attachment: HTML test case

Comment 10

[Duncan Loveday]

Attached a file that does seem to work live in the bug and recreate the problem I'm seeing.

The original, now-obsoleted test case seems to be rendered at different scales on different PCs, different versions of firefox and when loaded as HTTP:// or FILE:// but that's a separate issue.

Comment 11

[Duncan Loveday]

Filed bug 425826 for the different scaling observerd with file:// vs https:// although I can only reproduce that on one of my two PCs. Definitely unrelated to this bug - didn't regress at the same time (if indeed it is a regression).

Comment 12

[Duncan Loveday]

(In reply to comment #8)
(In reply to comment #10 para 2)
(In reply to comment #11)

The thing about scaling being different in file:// vs https:// is a red herring - however to reproduce this bug it's important NOT use page zooming as it is dependent on the scale things are drawn at. Press ctrl-0 to reset page zoom for the bugzilla domain.

Comment 13

[Robert Longson [:longsonr]]

Attachment: simpler testcase

we seem to get the bounding box right but the hittest area for text is too big as therefore is the covered region. The mouse active area goes to the top of the screen rather than just being over the text

Comment 14

[Duncan Loveday]

I think maybe this ought to be a blocker. My app is pretty much unusable since this bug appeared

Comment 15

[Robert Longson [:longsonr]]

Attachment: ok testcase

Comment 16

[Robert Longson [:longsonr]]

Attachment: this one doesn't work.

Comment 17

[Robert Longson [:longsonr]]

The difference between the ok testcase and the non-working one is that the viewBox for the former is "0 0 2258 2258", whereas the viewBox for the latter is "0 0 2259 2259".

The area the mouse changes to a link covers just the text in the ok testcase whereas in the non-working testcase, the link area goes all the way to the top of the screen. Somehow the y and/or height calculation has overflowed.

Comment 18

[Duncan Loveday]

For me, they both behave the same - broken but not in the way you describe. To get the alert I have to click in the left portion of the text, anywhere up to the "k" of "click" in "click me". With these tests I don't see the hyperlink area extending to the top of the screen.

I do see the hyperlink area extending to the top of the screen with this attachment https://bugzilla.mozilla.org/attachment.cgi?id=312680.

Comment 19

[Robert Longson [:longsonr]]

Resolution/screen size/browser window size dependent perhaps? Does increasing the viewBox size beyond 2259 break things eventually?

Comment 20

[Robert Longson [:longsonr]]

Hmm, both new testcases work now that I've restarted my browser.

Comment 21

[Robert Longson [:longsonr]]

(In reply to comment #18)
> I do see the hyperlink area extending to the top of the screen with this
> attachment https://bugzilla.mozilla.org/attachment.cgi?id=312680.
> 

For me, sometimes I run the browser and that works OK, and sometimes it doesn't.

Comment 22

[Duncan Loveday]

Yeah, drives you mad doesn't it ? Just now I observed both your last 2 attachments working correctly and the attachment at  https://bugzilla.mozilla.org/attachment.cgi?id=312680 not responding to clicks at all. Now I'm back to having to click on the left portion of the text.

In my real app I also observed successive runs of the same thing showing the bug and then not showing it.

Do you think there's something time dependent here ? I'm reminded slightly of bug 380516. Boris Zbarsky explained in comment 25 that reflow events can race with page load to give intermittent behaviours.

Comment 23

[Duncan Loveday]

Just now the attachment at https://bugzilla.mozilla.org/attachment.cgi?id=312680 responded to clicks in an area above the left portion of the rectangle extending upwards but not all the way to the top of the screen. About one third of the way up. You blink and something different happens.

Comment 24

[Robert Longson [:longsonr]]

When it goes wrong the cause is that cached font metric information is incorrect. Since it is cached, once it is wrong it stays wrong till the browser restarts.

I suspect an uninitialised variable somewhere rather than something time dependent.

Comment 25

[Duncan Loveday]

Got it.

I'm a bit wary of making any definite claims about this bug because no sooner do I post a comment than something different happens, BUT I just started a new browser three times with the window maximised and the attachment https://bugzilla.mozilla.org/attachment.cgi?id=312680 didn't respond to clicks which is correct behaviour I think. Then I did the same thing with the window about half height and half width and all three times I got the effect I described in comment #23. Then I went back to maximised and it got correct behaviour.

Comment 26

[Duncan Loveday]

....but then I repeated all that with some of the other attachments and got wrong the bug all the time.

Comment 27

[Damon Sicore (:damons)]

-'ing this as we really need a consistently reproducible test case.  Please
re-nom once we have one.

Comment 28

[Duncan Loveday]

(In reply to comment #27)
> -'ing this as we really need a consistently reproducible test case.  Please
> re-nom once we have one.
> 

It seems to be in the nature of this bug that it's hard to reproduce all the time but nonetheless the HTML attachment reproduces this consistently for me, so re-nominating.

Comment 29

[Damon Sicore (:damons)]

What happens when you back out bug 392233? 

Comment 30

[Robert Longson [:longsonr]]

Attachment: patch

If you display the html testcase first then my testcases show the issue.
If you don't display the html testcase first then my testcases don't show any issues. 

In the html testcase the cairo ctm is 0.01499 when gfxFont::SetupGlyphExtents is called. The cairo extents values come out scaled by the inverse of this so you get really big extents in gfxFont::Measure which are never scaled back again.

Are there any other tests I can run anywhere? Non-svg tests perhaps?

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 313149 [details] [diff] [review]
patch

You could manually check the testcases in bug 96041

Comment 32

[Robert Longson [:longsonr]]

Comment on attachment 313149 [details] [diff] [review]
patch

I see no difference in the testcases but I think html is always going to have an identity ctm unless it's in an SVG foreignObject, examining the testcases shows an identity ctm in all those cases.

Hopefully fairly low risk as I think it's hard to get html with a non-identity transformation matrix. 

Zooming doesn't change the transformation matrix for instance.

Comment 33

[Duncan Loveday]

Robert, after running the HTML test case I too see a the behaviour of your attachments changing at the 2258/2259 boundary. But what's worring me is that I also see both of the attachments misbehaving in a different way when I restart the browser and don't run the HTML test case. I see the effect I described in comment 18. Do you see that also ?

Comment 34

[Robert Longson [:longsonr]]

Yes, but that's not this bug. Needs a new one raising.

Comment 35

[Duncan Loveday]

Filed 426738. I'll try to get a regression range.

Comment 36

[Duncan Loveday]

(In reply to comment #32)
> 
> Zooming doesn't change the transformation matrix for instance.
> 

For me, if I start with a new browser, reset my zoom state with ctrl-0 and click on my original attachment https://bugzilla.mozilla.org/attachment.cgi?id=312321, it behaves itself but if I zoom out four times with ctrl-- it starts misbehaving.

Comment 37

[Damon Sicore (:damons)]

What's the risk associated with taking this patch?  Are the tests we have good enough to test for such regressions?  Comment #30 seems to imply we're not sure of regressions here.  Approval pending addressing these concerns.  

Comment 38

[Damon Sicore (:damons)]

Comment on attachment 313149 [details] [diff] [review]
patch

re-request approval once above questions addressed.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 313149 [details] [diff] [review]
patch

This is a very safe patch. The reason we don't have many tests covering this is that it will only affect SVG text; during normal browser operation the gfxContext will have the identity CTM so this patch will have no effect.

Furthermore the patch is "obviously correct", we completely understand this code.

Comment 40

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 313149 [details] [diff] [review]
patch

a1.9=beltzner

Comment 41

[Robert Longson [:longsonr]]

checked in

Comment 42

[Josh Aas]

I backed this out in case it caused the tbox orange we're seeing now.

Comment 43

[Josh Aas]

I don't think this caused the unit test failures.

Comment 44

[Dan Mills [:thunder]]

Yeah, I agree.  Bug 426501 was the cause.

Comment 45

[Robert Longson [:longsonr]]

checked in again.