893572 - Canvas2D crash [@mozilla::gfx::DrawTargetCG::StrokeRect]

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

if (isGradient(aPattern)) {
    // There's no CGContextClipStrokeRect so we do it by hand
    CGContextBeginPath(cg);
    CGContextAddRect(cg, RectToCGRect(aRect));
    CGContextReplacePathWithStrokedPath(cg);
    //XXX: should we use EO clip here?
    CGContextClip(cg);
    DrawGradient(cg, aPattern);
  } else {
    SetStrokeFromPattern(cg, mColorSpace, aPattern);
*   CGContextStrokeRect(cg, RectToCGRect(aRect));
  }



Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/fa6ef0b63025

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Al Billings [:abillings - ex-MoCo]]

Milan, who can we assign this to?

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

What Joe said.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

I believe this stopped crashing for me under ASAN in the nightly.  I don't have a window as to when it stopped, probably within the past couple of weeks.

Comment 5

[Christoph Diehl [:posidron]]

No, it is still reproducible with:

http://hg.mozilla.org/integration/mozilla-inbound/rev/f85f06761d52

Comment 6

[Al Billings [:abillings - ex-MoCo]]

Jeff, any updates here?

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Thought this was only crashing behind a pref, but now it does crash on the default profile.  The reason it "worked" for me was because I had switched to SkiaGL instead.  That one runs clean.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Right - there is a crash with the default configuration.  Setting gfx.canvas.azure.backends from the default cg to skia stops the crash.  If left at the default cg, we crash.  gfx.canvas.azure.accelerated setting is irrelevant - crashes with the default false and non-default true.  Yes, it's obvious from the original comment, but thought I'd mention it because I got caught on the pref change.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: This testcase is OK - fillRect seems to be OK.

This test should pass, with fillRect instead of strokeRect.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: fillRect + clearRect - also OK

This test is also OK - fillRect + clearRect

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: 893572.p

To build, this needs a patch from bug 903526 - the feedback is to see if it makes sense to just reject degenerate rectangles, and also whether to just do it in StrokeRect (the crash here) or also in FillRect and/or ClearRect even though they have no ASAN errors with the infinite values.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Al, a question - the patch for this looks like a performance improvement.  However, attaching it to a security bug, one can "see" how to trigger this crash (I don't know about exploiting it, though). Does it make sense to open a different (performance?) bug and make the fix against that one, or just do it in this one?

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Guard StrokeRect on OSX against "infinite" values

Just guarding StrokeRect - should the same guards be done in ClearRect and FillRect (no crash in those, it seems.)

Comment 14

[Al Billings [:abillings - ex-MoCo]]

I would open the perf cover bug.

That said, before this is checked in anywhere, you should set sec-approval? on the bug and fill out the template.

We don't really know how far back this goes?

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

I believe it's been around since January '12 when the Azure on OSX got enabled.  However, this is a crash in CoreGraphics itself, we are just working around it, and I don't know if that crash exists in all versions of OSX.  I'll get the patch, get the sec-approval, and we can then decide if it needs an uplift.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 790301 [details] [diff] [review]
Guard StrokeRect on OSX against "infinite" values

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

I wonder if this guard should be moved up to canvas? Could we have the same problem if the rectangle is constructed manually with a path.
Also this patch should include the test as a crash-test reftest.

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

Anything that calls CGContextStrokeRect will have a problem, but that's only the rect.  I believe you're asking if CGContextStrokePath would also have a problem - I will check and get the test done.

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

Path with "infinite" seems to be OK.  I'll add it to the same crash test.

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: 893572-testcase-line.html

I lied - it does crash with the path as well.  Here's the test.

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Catch the non-finite numbers on OS X that crash in the CG code.

Check for inf in paths, lines or rectangles guarding against crashes on OS X. Some of these methods are being called from multiple places, are public methods, so it seems we need to protect inside DrawTargetCG, rather than up in the canvas code, in case they get used elsewhere. Note this is only an issue for numbers larger than float max and smaller than double max, because those will get through the LenientFloat in the IDL.

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

Speaking of LenientFloat and IDL - that's bug 767933.

Comment 22

[Matt Wobensmith [:mwobensmith][:matt:]]

Review from [e-mail]?

Comment 23

[bhavana bajaj [:bajaj]]

I am tracking this because its a sec-crit but given where we are in the cycle for Fx24 and keeping in mind this bug is old enough and dates back to January '12, this will be a wontfix unless anyone can make a strong case to take it in our final two weeks of Beta and deems the patch is super safe to get it landed by Monday, which is when our next beta is going to build.

Comment 24

[Milan Sreckovic [:milan] (needinfo for best results)]

I think wontfix for the Beta is reasonable.  I'm 50/50 on whether we should put it in Aurora, I think we could decide to just let it ride the 26 train.  Having said that, how does this bug date back to January '12?

Comment 25

[bhavana bajaj [:bajaj]]

(In reply to Milan Sreckovic [:milan] from comment #24)
> I think wontfix for the Beta is reasonable. 
wontfix'ing for fx24.
>Got i from your comment  I'm 50/50 on whether we should
> put it in Aurora, I think we could decide to just let it ride the 26 train. 
> Having said that, how does this bug date back to January '12?

Got it from your comment above https://bugzilla.mozilla.org/show_bug.cgi?id=893572#c15, did I catch something wrong ?

Comment 26

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to bhavana bajaj [:bajaj] from comment #25)
> (In reply to Milan Sreckovic [:milan] from comment #24)
> > I think wontfix for the Beta is reasonable. 
> wontfix'ing for fx24.
> >Got i from your comment  I'm 50/50 on whether we should
> > put it in Aurora, I think we could decide to just let it ride the 26 train. 
> > Having said that, how does this bug date back to January '12?
> 
> Got it from your comment above
> https://bugzilla.mozilla.org/show_bug.cgi?id=893572#c15, did I catch
> something wrong ?

Fair enough, it probably has been around for that long.

Comment 27

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 792946 [details] [diff] [review]
Catch the non-finite numbers on OS X that crash in the CG code.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Fairly easily to crash, not sure if it can be exploited.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
If attached to the security bug, probably.

Which older supported branches are affected by this flaw?
This is a problem in CoreGraphics, so most of them.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
If we want to, it shouldn't be difficult to port.

How likely is this patch to cause regressions; how much testing does it need?
Low chance of regressions.  The patch changes the behaviour of the code that would have crashed before. Should not have impact on "normal" data.

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 792946 [details] [diff] [review]
Catch the non-finite numbers on OS X that crash in the CG code.

We can't check this in with tests, the tests will have to land after the fix ships. In fact we really ought to tell Apple about the CoreGraphics bug before checking in the tests so we might want to clone a separate "check in the tests" bug.

This also apparently needs the patch from bug 903526 so the ESR-17 and Aurora backports will have to contain that code as well (or if it applies as is then a branch-approval request in that bug).

Waiting for the roll-up branch patches before granting approval so we know they're ready to go in together (and also because I'm nervous about approving an hg bundle with tests in case someone else checks in without reading the comments).

Comment 29

[Milan Sreckovic [:milan] (needinfo for best results)]

Right - see also Comment 14; I was assuming we would spin up a separate bug, call it "performance" so that we don't draw attention to this being a security bug.  The fix would go in there, the tests would eventually (when?) get landed with this bug.  Or something like that?

Comment 30

[Daniel Veditz [:dveditz]]

I could approve that approach, yes

Comment 31

[Milan Sreckovic [:milan] (needinfo for best results)]

This "fake" bug 913614.  Please don't set dependencies between these two bugs.  Daniel, I will let you know when the fix only (same fix extracted from the patch above) is attached to bug 913614 and you can approve it in this bug before we get it checked in.

Comment 32

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Additional crashtests to catch this problem. The code fix is part of a bug 913614. Carry r=jmuizelaar.

This is the tests only part of the attachment 792946 [details] [diff] [review] as the code part is in bug 913614.

The security approval flag should only be set once we want these tests returned.

Comment 33

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Daniel Veditz [:dveditz] from comment #30)
> I could approve that approach, yes

There is now an attachment in bug 913614 with the code changes alone.  Daniel, please let me know in this bug if that attachment to bug 913614 is OK to land on central.

Comment 34

[Daniel Veditz [:dveditz]]

sec-approval+ on the patch in bug 913614

Comment 35

[Milan Sreckovic [:milan] (needinfo for best results)]

https://tbpl.mozilla.org/?tree=Try&rev=51b35d25c8e7

Comment 36

[Milan Sreckovic [:milan] (needinfo for best results)]

Fix landed in central under bug 913614 https://hg.mozilla.org/mozilla-central/rev/03dbb211fd6b

Comment 37

[Milan Sreckovic [:milan] (needinfo for best results)]

Bhavana, look for the aurora approval in bug 913614 - it's really for this bug, but we don't want to mention the existence of this bug in 913614.

Comment 38

[bhavana bajaj [:bajaj]]

(In reply to Milan Sreckovic [:milan] from comment #37)
> Bhavana, look for the aurora approval in bug 913614 - it's really for this
> bug, but we don't want to mention the existence of this bug in 913614.

Just approved ! Also setting flags for esr24 as the patch would be needed on that branch as well.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5706a9c6704a

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage due to dependencies not being nommed and approved for uplift...
https://hg.mozilla.org/releases/mozilla-aurora/rev/97e0d6a03696

Comment 41

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #40)
> Backed out for bustage due to dependencies not being nommed and approved for
> uplift...
> https://hg.mozilla.org/releases/mozilla-aurora/rev/97e0d6a03696

I don't think this one is meant to be checked in at all until 25 ships?  See the whiteboard.  Once 25 ships, 26 will be in beta, and we will then check this into 28 and uplift to 27 aurora and 26 beta.  At least, this is how I understood it, and this is why sec-approval is still a "?"

Comment 42

[u279076]

FWIW, I am not seeing this signature reported in the wild:
> https://crash-stats.mozilla.com/query/?query_type=simple&query=mozilla%3A%3Agfx%3A%3ADrawTargetCG%3A%3AStrokeRect

Comment 43

[Andrew McCreight [:mccr8]]

Looks like bug 913614 landed in 25.

Comment 44

[Andrew McCreight [:mccr8]]

Marking fixed because it is fixed on trunk.

Comment 45

[Andrew McCreight [:mccr8]]

This still needs to be landed on ESR-17 and ESR-24, it looks like.  (If we're still landing things on 17...)

Comment 46

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Andrew McCreight [:mccr8] from comment #44)
> Marking fixed because it is fixed on trunk.

Not completely - the tests are not checked in anywhere.  We're waiting for the fix to go public with 25, before we return the tests.  So, there still needs to be something that happens to this bug - perhaps we can leave it open so that we don't lose track of it?

Comment 47

[Andrew McCreight [:mccr8]]

Sure.

Comment 48

[Al Billings [:abillings - ex-MoCo]]

(In reply to Andrew McCreight [:mccr8] from comment #45)
> This still needs to be landed on ESR-17 and ESR-24, it looks like.  (If
> we're still landing things on 17...)

We're still supporting both (there is a two version overlap) so we should take it on both.

Comment 49

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash FF25, 2013-07-11.
Verified fixed FF25, FF26, FF27, 2013-10-01.

Comment 50

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 800926 [details] [diff] [review]
Additional crashtests to catch this problem.  The code fix is part of a bug 913614.  Carry r=jmuizelaar.

Setting sec-approval+ per comment 34 but tests shouldn't land until after 10/29 when Firefox 25 ships.

Comment 51

[David Bolter [:davidb] (NeedInfo me for attention)]

Hi Milan, can someone land this fix (from bug 913614) on ESR? (Looking at the flags I assume that didn't happen yet).

Comment 52

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to David Bolter [:davidb] from comment #51)
> Hi Milan, can someone land this fix (from bug 913614) on ESR? (Looking at
> the flags I assume that didn't happen yet).

We could put it together, but I was thinking that comment 15 and comment 25 were having us skip the 24ESR.  Do we want it there?

Comment 53

[Milan Sreckovic [:milan] (needinfo for best results)]

I guess we do because landing the tests in 25 gives people a hint how to break 24?

Comment 54

[Al Billings [:abillings - ex-MoCo]]

ESR24 should take important security fixes for Firefox 25 (and 26, 27, etc. until end of life). 

This was sec-critical, right?

Comment 55

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: 24 ESR Patch for this and the dependent bugs. Carry r=jmuizelaar.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
It is sec-critical.

User impact if declined: 
SEcurity vulnerability 

Fix Landed on Version: 25

Risk to taking this patch (and alternatives if risky): 
After 25 ships, we will be landing a test that reproduces this crash on 24.

String or UUID changes made by this patch: None

Comment 56

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Al Billings [:abillings] from comment #54)
> ESR24 should take important security fixes for Firefox 25 (and 26, 27, etc.
> until end of life). 
> 
> This was sec-critical, right?

I attached the 24 ESR patch - it should wait until after 25 ships, as it contains the same set of tests that expose the vulnerability.

Comment 57

[Al Billings [:abillings - ex-MoCo]]

The whole point was to ship the fix in ESR24 at the same time as we ship Firefox 25 though. Landing this after 25 ships kind of defeats the point of not zero daying ESR24 with a sec-critical. I'm less concerned about the tests than making sure the underlying vuln is fixed in ESR24.

Comment 58

[Milan Sreckovic [:milan] (needinfo for best results)]

You're right, I should have said "not before 25 ships", so at the same time would work perfectly!

Comment 59

[Al Billings [:abillings - ex-MoCo]]

Let's get this checked into ESR24 ASAP.

Comment 60

[Al Billings [:abillings - ex-MoCo]]

We seem to have missed the windows for ESR24?

Comment 61

[Al Billings [:abillings - ex-MoCo]]

And ESR17 as well.

Any comments from release management?

Comment 62

[Al Billings [:abillings - ex-MoCo]]

Release Management says in IRC that you can land this for ESR17 and ESR24 if you land it today, Milan.

Comment 63

[Andrew McCreight [:mccr8]]

I'm currently cloning ESR24 for landing the patch https://bugzilla.mozilla.org/attachment.cgi?id=816090

It sounds like I should land it without the tests, so I'll just land the code changes.

Is it expected to cleanly apply to ESR17?

Comment 64

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/b6aa22c418f4

Comment 65

[Andrew McCreight [:mccr8]]

I don't know if this will work or not, but at least it applies cleanly, which gives me hope.

https://hg.mozilla.org/releases/mozilla-esr17/rev/eeab87c4c2db

(I landed without patches, and with the same checkin message as was used previously for bug 913614)

Comment 66

[Al Billings [:abillings - ex-MoCo]]

Thanks for landing these.

Comment 67

[Milan Sreckovic [:milan] (needinfo for best results)]

Thanks indeed.  I'm in CST at a work week (this week), so I didn't see this exchange until now.

Comment 68

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified on shipping 17.0.10esr and 24.1esr.

Comment 69

[Al Billings [:abillings - ex-MoCo]]

It is after 10/29. Can we land tests?

Comment 70

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Al Billings [:abillings] from comment #69)
> It is after 10/29. Can we land tests?

Yes please!

Comment 71

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb843a74b5b

Comment 72

[Ed Morley [:emorley]]

The tests crash... this sadly isn't fixed after all :-s

https://tbpl.mozilla.org/php/getParsedLog.php?id=30334956&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=30334722&tree=Mozilla-Inbound

{
09:15:43  WARNING -  PROCESS-CRASH | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/gfx/tests/crashtests/893572-2.html | application crashed [@ CoreGraphics + 0x188b9b]
09:15:43     INFO -  Crash dump filename: /var/folders/gc/nk_nqbn10l9_5k6rmxmkt_9400000w/T/tmpj_RO4Y.mozrunner/minidumps/94AB241A-5616-4AA8-BC52-D541227F2D2D.dmp
09:15:43     INFO -  Operating system: Mac OS X
09:15:43     INFO -                    10.7.2 11C74
09:15:43     INFO -  CPU: amd64
09:15:43     INFO -       family 6 model 23 stepping 10
09:15:43     INFO -       2 CPUs
09:15:43     INFO -  Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
09:15:43     INFO -  Crash address: 0x5fc39d1d
09:15:43     INFO -  Thread 0 (crashed)
09:15:43     INFO -   0  CoreGraphics + 0x188b9b
09:15:43     INFO -      rbx = 0x0000000000000125   r12 = 0x0000000000000000
09:15:43     INFO -      r13 = 0x000000013c80a128   r14 = 0x0000000000000001
09:15:43     INFO -      r15 = 0x00007fff5fc39d1d   rip = 0x00007fff8de55b9b
09:15:43     INFO -      rsp = 0x00007fff5fbf9c80   rbp = 0x00007fff5fbf9cc0
09:15:43     INFO -      Found by: given as instruction pointer in context
09:15:43     INFO -   1  CoreGraphics + 0x188701
09:15:43     INFO -      rip = 0x00007fff8de55702   rsp = 0x00007fff5fbf9cd0
09:15:43     INFO -      rbp = 0x00007fff5fbf9d40
09:15:43     INFO -      Found by: stack scanning
09:15:43     INFO -   2  CoreGraphics + 0x1882d4
09:15:43     INFO -      rip = 0x00007fff8de552d5   rsp = 0x00007fff5fbf9d50
09:15:43     INFO -      rbp = 0x00007fff5fbf9d80
09:15:43     INFO -      Found by: stack scanning
09:15:43     INFO -   3  CoreGraphics + 0x187e4b
09:15:43     INFO -      rip = 0x00007fff8de54e4c   rsp = 0x00007fff5fbf9d90
09:15:43     INFO -      rbp = 0x00007fff5fbf9e10
09:15:43     INFO -      Found by: stack scanning
09:15:43     INFO -   4  libRIP.A.dylib + 0x5f1e
09:15:43     INFO -      rip = 0x00007fff8e7e0f1f   rsp = 0x00007fff5fbf9e20
09:15:43     INFO -      rbp = 0x00007fff5fbf9ed0
09:15:43     INFO -      Found by: stack scanning
09:15:43     INFO -   5  libRIP.A.dylib + 0x573c
09:15:43     INFO -      rip = 0x00007fff8e7e073d   rsp = 0x00007fff5fbf9ee0
09:15:43     INFO -      rbp = 0x00007fff5fbf9f50
09:15:43     INFO -      Found by: stack scanning
09:15:43     INFO -   6  libRIP.A.dylib + 0x8b4a
09:15:43     INFO -      rip = 0x00007fff8e7e3b4b   rsp = 0x00007fff5fbf9f60
09:15:43     INFO -      rbp = 0x00007fff5fbfa080
09:15:43     INFO -      Found by: stack scanning
}

Backed the tests out:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1a92a46ff6be

Comment 73

[Milan Sreckovic [:milan] (needinfo for best results)]

Let me check.  There were recent changes to the original fix, may have caused this problem.

Comment 74

[Milan Sreckovic [:milan] (needinfo for best results)]

The tests pass in release, beta and aurora, but fail on central.  I'll find a regression range.

Comment 75

[Milan Sreckovic [:milan] (needinfo for best results)]

The tests stopped passing: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=47c8e9b16918&tochange=b4143e04bea1

Comment 76

[Milan Sreckovic [:milan] (needinfo for best results)]

The regression comes from bug 922942 - https://bug922942.bugzilla.mozilla.org/attachment.cgi?id=826199

Comment 77

[Milan Sreckovic [:milan] (needinfo for best results)]

On a side note, BaseRect::IsFinite() as of recently works on windows.

Comment 78

[Matt Woodrow (:mattwoodrow)]

Are you sure it's that change?

The change was mainly to gfxContext, which shouldn't be used by canvas at all.

The only other change was adding two IsFinite checks to DrawTargetCG, and it looks like canvas only calls those functions if we have shadows, or are drawing a focus ring.

Comment 79

[Bas Schouten (:bas.schouten)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #78)
> Are you sure it's that change?
> 
> The change was mainly to gfxContext, which shouldn't be used by canvas at
> all.
> 
> The only other change was adding two IsFinite checks to DrawTargetCG, and it
> looks like canvas only calls those functions if we have shadows, or are
> drawing a focus ring.

I agree this seems extremely unlikely..

Comment 80

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Bas Schouten (:bas.schouten) from comment #79)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #78)
> > Are you sure it's that change?
> > ...
> 
> I agree this seems extremely unlikely..

And yet... the crash goes away if you back out this change from PathCG::GetBounds function :)

+  if (!bounds.IsFinite()) {
+    return Rect();
+  }

The comment above says:
//XXX: what should these functions return for an empty path?
// currently they return CGRectNull {inf,inf, 0, 0}

except that with the above change, we start returning (0,0,0,0) instead.  Seems that somewhere down the line, having a (0,0,0,0) rectangle for that path that contains infinite causes problems.

Doing this:

if (!bounds.IsFinite()) {
  return bounds;
}

actually makes this particular crash go away.  Matt, what do you think?  Is that a reasonable change?

Comment 81

[Milan Sreckovic [:milan] (needinfo for best results)]

Same question for D2D.  We return 0 sized bounds for a path that contains infinity, which seems dangerous.

Comment 82

[Matt Woodrow (:mattwoodrow)]

If it passes all existing tests, then it seems fine.

Comment 83

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #82)
> If it passes all existing tests, then it seems fine.

Nope, it regresses test_text_selection.html.  I'll look at this a bit, but may need to pass it back.

Comment 84

[Bas Schouten (:bas.schouten)]

Clearing the flag as this is being looked into :)

Comment 85

[Bas Schouten (:bas.schouten)]

Attachment: tmp

I wonder if this patch will have more luck.

Comment 86

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8337770 [details] [diff] [review]
tmp

F+ meaning that this fixes the crash for me.  Pushed to try even as this is security bug, as all that's left are the tests.
https://tbpl.mozilla.org/?tree=Try&rev=ee2916dc1589

Comment 87

[Matt Woodrow (:mattwoodrow)]

Looks like this doesn't pass tests either :(

Comment 88

[Milan Sreckovic [:milan] (needinfo for best results)]

Not sure about that.  The test_text_selection has had trouble passing last time I did a try run and were failing for me locally with or without this change.  Haven't looked at the rest.

Comment 89

[Matt Woodrow (:mattwoodrow)]

But it must be passing on the tbpl machines without this patch, since we're definitely green on m-c.

Comment 90

[Milan Sreckovic [:milan] (needinfo for best results)]

Indeed.  All I'm saying is that the same test fails for me locally without the patch.

Comment 91

[Milan Sreckovic [:milan] (needinfo for best results)]

Or maybe not. Never mind.

Without the change, the new crashtests from this bug fail.
With the change from comment 85, the new crashtests from this bug succeed.  The mochitest 1 ( test_text_selection.html) fails.

Comment 92

[Milan Sreckovic [:milan] (needinfo for best results)]

The problem is rounding inf/nan rectangles into integer ones for SVG to use.

Comment 93

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Watch when converting float to int rectangles and infinites.

This is just putting the check in the method that was causing the tests to fail.
This backs out part of the bug 922942 that was meant to get the SVG tests to pass.
We may want to put warnings or asserts in BaseRect::Round* methods to notify us of non-finite scenarios, and we may even want to put the checks in there - seems like we may as well wait with the checks, see how common this is.

https://tbpl.mozilla.org/?tree=Try&rev=71145173d1fd

Comment 94

[Bas Schouten (:bas.schouten)]

Comment on attachment 8338677 [details] [diff] [review]
Watch when converting float to int rectangles and infinites.

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

::: gfx/2d/PathCG.cpp
@@ +289,5 @@
>  {
>    //XXX: are these bounds tight enough
>    Rect bounds = CGRectToRect(CGPathGetBoundingBox(mPath));
>    if (!bounds.IsFinite()) {
> +    return bounds;

Technically I wonder if we should still transform here, since [x,inf] can still be translated to [x+a,inf] validly. Also I'm wondering whether we should still watch for NaN or not.

Comment 95

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Bas Schouten (:bas.schouten) from comment #94)
> Comment on attachment 8338677 [details] [diff] [review]
> Watch when converting float to int rectangles and infinites.
> 
> Review of attachment 8338677 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/PathCG.cpp
> @@ +289,5 @@
> >  {
> >    //XXX: are these bounds tight enough
> >    Rect bounds = CGRectToRect(CGPathGetBoundingBox(mPath));
> >    if (!bounds.IsFinite()) {
> > +    return bounds;
> 
> Technically I wonder if we should still transform here, since [x,inf] can
> still be translated to [x+a,inf] validly. Also I'm wondering whether we
> should still watch for NaN or not.

On the NaN side - are you saying we should only watch for NaN, or not watch for NaN at all, and just watch for inf?

On the transform side - I could leave the transform in there; in the test case, we start with (inf,inf,0,0), which, after the transform, ends up (inf,inf,inf,inf).

Comment 96

[Ryan VanderMeulen [:RyanVM]]

Comment 95 makes it seem like this isn't ready to land yet? Also, the Try push in comment 93 is showing OSX reftest failures.

Comment 97

[Al Billings [:abillings - ex-MoCo]]

Any updates here, Milan?

Comment 98

[Milan Sreckovic [:milan] (needinfo for best results)]

Not a lot.  The try fails, but locally things work (on 10.8 debug), so it's a bit more of a pain to figure out what's going on.

Comment 99

[Milan Sreckovic [:milan] (needinfo for best results)]

https://tbpl.mozilla.org/?tree=Try&rev=0bffe8810a58 seems to be green.

Comment 100

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: The tests that catch the problem. Backout part of 922942. Carry r=bas, mattwoodrow

Address comment #95, add the tests, back out part of 922942 that caused the tests to regress.  https://tbpl.mozilla.org/?tree=Try&rev=de1dbbc4e939

Comment 101

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3735493e9aa8

Pretty sure we're going to need this on Aurora as well given that bug 922942 landed on Fx28.

Comment 102

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8349026 [details] [diff] [review]
The tests that catch the problem.  Backout part of 922942.  Carry r=bas, mattwoodrow

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Combination of the original problem and bug 922942
User impact if declined: 
We have part of the original bug broken by 922942 - security risk.
Testing completed (on m-c, etc.): m-c.
Risk to taking this patch (and alternatives if risky): 
sec-critical issue.
String or IDL/UUID changes made by this patch: n/a

Comment 103

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3735493e9aa8

Comment 104

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bd169dd60c78

I plan to push the test to beta/b2g26/esr24 as a ride-along as well.

Comment 105

[Ryan VanderMeulen [:RyanVM]]

Uplifted the tests to the other branches:
https://hg.mozilla.org/releases/mozilla-beta/rev/0d6db3ff09bb
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/4acf03d70eab
https://hg.mozilla.org/releases/mozilla-esr24/rev/f88df92166ae