1340627 - Update skia to version 59

Status: RESOLVED FIXED

(Core :: Graphics, enhancement, P3)

Description

[Ryan VanderMeulen [:RyanVM]]

(In reply to Tom Ritter [:tjr] from bug 1338658 comment #0)
> This is a (semi-)automated bug making you aware that there is an available
> upgrade for an embedded third-party library.
> 
> skia is currently at version 55 in mozilla-central, and the latest version
> of the library released is 58.
> 
> I fetched the latest version of the library from
> https://skia.googlesource.com/skia/+/master/include/core/SkMilestone.h.
> 
> You can leave this bug open, and it will be updated if a newer version of
> the library becomes available. If you close it as WONTFIX, please indicate
> if you do not wish to receive any future bugs upon new releases of the
> library.

Per discussion with lsalzman, we're not ready to update yet, but getting this bug on file to track it when we are.

Comment 1

[Tom Ritter [:tjr]]

Skia 58 has been released.

Comment 2

[Lee Salzman [:lsalzman]]

Still waiting on Skia upstream to potentially incorporate some fixes/features before I move on this.

Comment 3

[Tom Ritter [:tjr]]

Skia 59 has been released.

Comment 4

[Lee Salzman [:lsalzman]]

Attachment: Updated moz-skia sources to m59

Nothing too controversial here in terms of my new commits. Just a lot of churn updating old patches to work with m59, and then some new fixes to deal with build issues, and some compat things like the use of std::chrono features that we don't support.

Comment 5

[Lee Salzman [:lsalzman]]

Attachment: update moz.build for Skia m59

The main change here is the migration from gyp to gn. So all our gyp-related scripts get to go away... that's the good part.

The most effective way to get gn to do what we want here is to use "gn gen" to first generate the build files for the desired platform, then use "gn desc" to list out the manifests thereof for specific targets (like :effects, :gpu, :pdf, etc).

In the process replaced use of deprecated os.system with the subprocess module, which is ordained way to do such things now.
 
I also cleaned up the generation of opt sources by using the specific optimization targets (:sse2, :armv7, etc) instead of just walking the entire directory which was not terribly safe. This makes binning the files into the right sets in the sources dictionary easier to, since we no longer need to do it in post - they go right to the sorted sets where they need to go from the beginning.

Also of note, NEON is now always used throughout the build. The groundwork for this was taken care of when we migrated all non-NEON ARM users to 52 indefinitely and not letting them progress to 53+. So we've been clear to make that change for a while, and Milan has given the go-ahead for it.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8865116 [details] [diff] [review]
update moz.build for Skia m59

Probably good to get a build peer to look at this.

Comment 7

[Lee Salzman [:lsalzman]]

Attachment: fix Moz2d for Skia m59

A bunch of fun DrawTargetSkia changes herein...

They no longer support refcounting SkCanvas anymore, and their ordained way of managing it is with std::unique_ptr. BUT, SkSurface already owns the std::unique_ptr to it, so if you get an SkCanvas from the SkSurface, you're supposed to just keep the SkSurface around instead, and just point directly to the SkCanvas. We already keep around the SkSurface, so main hassle here is just getting rid of the sk_sp (shared pointer) junk. The only exception to this being the case of SkCanvas::MakeRasterDirect, which creates a canvas that is not owned by an SkSurface, so we must use std::unique_ptr there.

Simplified LockBits implementation to migrate away from SkCanvas::getTopDevice usage which is not really public anymore.

Some fixup to deal with gradient shader instantiation possibly returning null, so need to just draw transparent instead there to make some reftests pass.

Other bugbear: no more replayClips. So sanest idea here was to use what little public clip API there is left - isClipRect() and getDeviceClipBounds(). If the clip is a rectangle, we can just get the bounds and apply that to the CG context and call it good. For more complicated cases, we instead now have to use saveLayer, draw text to the new layer, then restore, which will blend back the layer with whatever complicated clips exist in the SkCanvas. At least for the common case (a simple clip rect), this should be fast, and relies only on public APIs.

Happy stuff - SkCanvas now has a way to tell saveLayer to just initialize the new layer with the contents of the previous layer, so all the annoyance of creating a handrolled CopyLayerImageFilter I can now just delete from the code.

Random little bits of API churn in terms of function names...

Comment 8

[Lee Salzman [:lsalzman]]

Attachment: fix SkiaGL glue for Skia m59

Had to add in the glPolygonMode function to GLContext as Skia added this as a requirement for instantiating GL interface.

Comment 9

[Lee Salzman [:lsalzman]]

Attachment: fix layers for Skia m59

Just some small fix-ups to deal with the above-mentioned issue that SkCanvas is no longer refcounted.

Comment 10

[Lee Salzman [:lsalzman]]

Attachment: fix Skia NPAPI for Skia m59

More cleanup to deal with SkCanvas refcounting fallout. About the sanest thing we do here is to treat the ANPCanvas as the owner, which for our remaining use-cases (Flash plugin) should be safe.

Comment 11

[Lee Salzman [:lsalzman]]

Attachment: fix thebes for Skia m59

Yet more SkCanvas refcounting fallout. In this case, the document, which PrintTargetSkPDF already manages through sk_sp, owns the returned SkCanvas from beginPage, so we just directly point to it instead of having to use std::unique_ptr.

Comment 12

[Lee Salzman [:lsalzman]]

Attachment: reftest fuzzing for update to Skia m59

Lots of reftest fuzzing to deal with the fact that Skia has done a lot of mucking around with path antialiasing since our last update. Fuzz all the things.

Comment 13

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8865117 [details] [diff] [review]
fix Moz2d for Skia m59

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

With minor nits

::: gfx/2d/DrawTargetSkia.cpp
@@ +1092,5 @@
>  // SetupCGContext.
>  CGContextRef
>  DrawTargetSkia::BorrowCGContext(const DrawOptions &aOptions)
>  {
> +  bool needLayer = mCanvas->isClipEmpty() && !mCanvas->isClipRect();

Why do we need a layer if the clip is empty? How come the condition here isn't just if the clip is more complex than a rect?

@@ +1172,5 @@
>    CGContextRestoreGState(aCGContext);
> +
> +  bool needLayer = !mCanvas->isClipEmpty() && !mCanvas->isClipRect();
> +  if (needLayer) {
> +    if (mCG) {

do we need to release mCG here? Looks like in BorrowCGContext, we'll already release the context here.

@@ +1779,5 @@
>    }
>  
>    mCanvas->save();
>    mCanvas->setMatrix(SkMatrix::MakeTrans(SkIntToScalar(aDestination.x), SkIntToScalar(aDestination.y)));
> +  mCanvas->clipRect(SkRect::MakeIWH(aSourceRect.width, aSourceRect.height), SkClipOp::kReplace_deprecated);

Uhh, what's supposed to be the proper clip op? kReplace_deprecated doesn't sound very good. Why isn't this kIntersect like the other uses below?

Comment 14

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8865120 [details] [diff] [review]
fix Skia NPAPI for Skia m59

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

::: other-licenses/skia-npapi/SkANP.h
@@ +59,4 @@
>      }
>  
>      ~ANPCanvas() {
> +        delete skcanvas;

hoorah for deletion of raw pointer! Just make skcanvas here private please.

Comment 15

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8865122 [details] [diff] [review]
fix thebes for Skia m59

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

::: gfx/thebes/PrintTargetSkPDF.h
@@ +58,5 @@
>    // The stream that the SkDocument outputs to.
>    UniquePtr<SkWStream> mOStream;
>  
>    // The current page's SkCanvas and its wrapping DrawTarget:
> +  SkCanvas* mPageCanvas;

nit: Just say the documents own the mPageCanvas and mRefCanvas.

Comment 16

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8865236 [details] [diff] [review]
reftest fuzzing for update to Skia m59

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

<3 Fuzz

Comment 17

[Lee Salzman [:lsalzman]]

(In reply to Mason Chang [:mchang] from comment #13)
> Comment on attachment 8865117 [details] [diff] [review]
> fix Moz2d for Skia m59
> 
> Review of attachment 8865117 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> With minor nits
> 
> ::: gfx/2d/DrawTargetSkia.cpp
> @@ +1092,5 @@
> >  // SetupCGContext.
> >  CGContextRef
> >  DrawTargetSkia::BorrowCGContext(const DrawOptions &aOptions)
> >  {
> > +  bool needLayer = mCanvas->isClipEmpty() && !mCanvas->isClipRect();
> 
> Why do we need a layer if the clip is empty? How come the condition here
> isn't just if the clip is more complex than a rect?

Typo. Meant !mCanvas->isClipEmpty(). Fixed that.

> @@ +1172,5 @@
> >    CGContextRestoreGState(aCGContext);
> > +
> > +  bool needLayer = !mCanvas->isClipEmpty() && !mCanvas->isClipRect();
> > +  if (needLayer) {
> > +    if (mCG) {
> 
> do we need to release mCG here? Looks like in BorrowCGContext, we'll already
> release the context here.

Since the mCanvas->restore() below it destroys the layer data the CG context referred to,
I try to make sure the CG context doesn't stick around after the layer goes bye-bye. That
way the context won't reused. Commented it as such.

> @@ +1779,5 @@
> >    }
> >  
> >    mCanvas->save();
> >    mCanvas->setMatrix(SkMatrix::MakeTrans(SkIntToScalar(aDestination.x), SkIntToScalar(aDestination.y)));
> > +  mCanvas->clipRect(SkRect::MakeIWH(aSourceRect.width, aSourceRect.height), SkClipOp::kReplace_deprecated);
> 
> Uhh, what's supposed to be the proper clip op? kReplace_deprecated doesn't
> sound very good. Why isn't this kIntersect like the other uses below?

CopySurface unfortunately has the spec'd behavior that it bypasses clip. For now it's not very easy or feasible
to change that until we do a more thorough investigation of what would break and how. For now Skia still supports
the Replace clip-op, but they are subtly hinting that they may not in the future. I'm just kicking the can down
the road on this one because solving it is a bit annoying without involvement from Skia guys possibly... or worse,
auditing every use of CopySurface in our codebase to not require the behavior. :/

Comment 18

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8865116 [details] [diff] [review]
update moz.build for Skia m59

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

This seems reasonable. Since this isn't actually run during build time, I'll let ted do a post-commit review and we can fix up anything afterwards.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af6f19870b2a
part 1 - update Skia source to m59. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d4ec7d38a00
part 2 - update moz.build for Skia m59. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/75fb0d9858a9
part 3 - fix Moz2d for Skia m59. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdbd5d281287
part 4 - fix thebes for Skia m59. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/026aadd76d06
part 5 - fix SkiaGL glue for Skia m59. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cb4bceb8d79
part 6 - fix layers for Skia m59. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/c691e2ab6a0c
part 7 - fix Skia NPAPI for Skia m59. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/f152be1fadb7
part 8 - reftest fuzzing for update to Skia m59. r=mchang

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b1371055c7f
clobber for Skia update. r=me

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/af6f19870b2a
https://hg.mozilla.org/mozilla-central/rev/0d4ec7d38a00
https://hg.mozilla.org/mozilla-central/rev/75fb0d9858a9
https://hg.mozilla.org/mozilla-central/rev/fdbd5d281287
https://hg.mozilla.org/mozilla-central/rev/026aadd76d06
https://hg.mozilla.org/mozilla-central/rev/3cb4bceb8d79
https://hg.mozilla.org/mozilla-central/rev/c691e2ab6a0c
https://hg.mozilla.org/mozilla-central/rev/f152be1fadb7
https://hg.mozilla.org/mozilla-central/rev/0b1371055c7f

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Attachment: More reftest fuzzing

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Attachment: More reftest fuzzing

Whoops, typed 255 instead of 225 in the last patch. For the record you can see the reftest failures at https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=ebbcdaa5b5802ecd39624dd2acbdda8547b8384d&filter-searchStr=quantum

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f151e6ae195
Follow-up to fuzz a reftest when webrender is enabled. r=lsalzman

Comment 25

[Mike de Boer [:mikedeboer]]

This patch makes Firefox unusable on - at least - OSX 10.12.4 due to abundant layout glitches in browser chrome and content.

I'm requesting a backout.

Comment 26

[Jonathan Kew [:jfkthame]]

(In reply to Mike de Boer [:mikedeboer] from comment #25)
> This patch makes Firefox unusable on - at least - OSX 10.12.4 due to
> abundant layout glitches in browser chrome and content.

Just to confirm, I'm seeing the same thing -- just spent some time bisecting locally and confirmed this is the bug where things start breaking.

Comment 27

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

We should back it out, but can we have a bit more detail on the STR?

Comment 28

[Mike de Boer [:mikedeboer]]

STR:

./mach build
./mach run

on OSX.

Comment 29

[Mike de Boer [:mikedeboer]]

If you don't see the glitches right away, try resizing the browser window and/ or opening a few tabs.

Pic: https://www.evernote.com/l/APsP8q1mHQtL8Ki0CMQA9tCsjRUJj1tHb48

Comment 30

[Panos Astithas (he/him) [:past] (please ni?)]

Milan, Mike is in Toronto Commons if you want to have a hands on experience.

Comment 31

[Kartikaya Gupta (email:[e-mail])]

I'm mostly unrelated to this bug, so dropping needinfo on me.

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/61fe0c6efb5e

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Oops, that was only the followup patch.

Rest backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/761b3f55f10f3df16601ce62e4d15b0c558c0cfb

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Merged the backout to central so we can re-spin nightlies: https://hg.mozilla.org/mozilla-central/rev/ce2218406119c36a551e3faea4e192186ee46cc5

Comment 35

[Mason Chang [Inactive] [:mchang]]

Attachment: Properly push / pop skia saved layers during borrow/return cg context

After saving a layer in Skia, some of the clip state gets reset in SkCanvas. Thus, the needLayer query in ReturnCGContext was always returning false since the SkCanvas::isClipRect() was returning true, meaning we'd never pop a layer.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ded74baeaf2
part 1 - update Skia source to m59. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b2683a840f3
part 2 - update moz.build for Skia m59. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/abffc78e1cf6
part 3 - fix Moz2d for Skia m59. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/7134018d3724
part 4 - fix thebes for Skia m59. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/84fd6b2bfaf6
part 5 - fix SkiaGL glue for Skia m59. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/b567eb8d5d39
part 6 - fix layers for Skia m59. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/a874592cff0a
part 7 - fix Skia NPAPI for Skia m59. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebc9a119bcf8
part 8 - reftest fuzzing for update to Skia m59. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/64c183fa4dd3
clobber for Skia update. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/2477a27a95a9
Follow-up to fuzz a reftest when webrender is enabled. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fcd9ee5ccb5
Properly push/pop skia saved layers during borrow/return cg context. r=lsalzman

Comment 37

[Mike de Boer [:mikedeboer]]

The inevitable question here: how could Firefox break this badly, but still have these changesets merged into m-c? IOW: can we have a test that goes orange when the UI goes bonkers?
I _know_ that this happens _very_ rarely, but seeing this, bisecting, requesting backout and the multiple clobber builds did destroy a rather high amount of hours I couldn't spend on Photon. (Apology for the perhaps grumpy tone; I'm a wee bit tired ;-) )

Comment 38

[Ryan VanderMeulen [:RyanVM]]

That's a completely fair question, Mike. And one that I was intending to ask myself had you not beat me to it! Lee, is there any kind of test we could add that would have caught the issue that led to the backout?

Comment 39

[Lee Salzman [:lsalzman]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #38)
> That's a completely fair question, Mike. And one that I was intending to ask
> myself had you not beat me to it! Lee, is there any kind of test we could
> add that would have caught the issue that led to the backout?

I myself assumed that since this had manage to survive the entire test suite unscathed on try several times over, where we already have a bunch of mochitests and reftests for browser chrome, that, well, it was working. Unfortunately, I'm not familiar with the specifics of the macOS widget code enough to understand why none of these possible failures were getting triggered at all.

Mason or Markus, do you have possibly ideas on why this might be the case? This is definitely somewhat of a mystery.

Comment 40

[Carsten Book [:Tomcat]]

have to backout this changes because they block a m-i to m-c merge like

erging layout/reftests/image-element/reftest.list
merging layout/reftests/svg/reftest.list
warning: conflicts while merging editor/reftests/reftest.list! (edit, then use 'hg resolve --mark')
warning: conflicts while merging layout/reftests/box-shadow/reftest.list! (edit, then use 'hg resolve --mark')
warning: conflicts while merging layout/reftests/bugs/reftest.list! (edit, then use 'hg resolve --mark')
warning: conflicts while merging layout/reftests/forms/select/reftest.list! (edit, then use 'hg resolve --mark')
warning: conflicts while merging layout/reftests/image-element/reftest.list! (edit, then use 'hg resolve --mark')
1468 files updated, 8 files merged, 371 files removed, 5 files unresolved
use 'hg resolve' to retry unresolved file merges or 'hg update -C .' to abandon

Comment 41

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91d70dad8c9c
Backed out changeset 1fcd9ee5ccb5 
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cc7ff8d300a
Backed out changeset 2477a27a95a9 
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e8f42ad6a16
Backed out changeset 64c183fa4dd3 
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6cf47e59877
Backed out changeset ebc9a119bcf8 
https://hg.mozilla.org/integration/mozilla-inbound/rev/05abcba1e2b6
Backed out changeset a874592cff0a 
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bf5043f1cd8
Backed out changeset b567eb8d5d39 
https://hg.mozilla.org/integration/mozilla-inbound/rev/c275b265fe5c
Backed out changeset 84fd6b2bfaf6 
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbc5f736d37c
Backed out changeset 7134018d3724 
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b073f27abb2
Backed out changeset abffc78e1cf6 
https://hg.mozilla.org/integration/mozilla-inbound/rev/8386de5a73d7
Backed out changeset 1b2683a840f3 
https://hg.mozilla.org/integration/mozilla-inbound/rev/904dbd853bac
Backed out changeset 0ded74baeaf2 for blocking m-i to m-c merge

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aa2f6aa8053
part 1 - update Skia source to m59. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbb70e16727b
part 2 - update moz.build for Skia m59. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/306f65d0a9eb
part 3 - fix Moz2d for Skia m59. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dc2a529c1a9
part 4 - fix thebes for Skia m59. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb088b241376
part 5 - fix SkiaGL glue for Skia m59. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d02032c3c3b
part 6 - fix layers for Skia m59. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/a97b1c46e56e
part 7 - fix Skia NPAPI for Skia m59. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/59b3cd42b4b5
clobber for Skia update. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ba1d4e0178a
part 8 - reftest fuzzing for update to Skia m59. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ebe08d1aa92
Follow-up to fuzz a reftest when webrender is enabled. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/e06077091df1
Properly push/pop skia saved layers during borrow/return cg context. r=lsalzman

Comment 43

[Lee Salzman [:lsalzman]]

(In reply to Carsten Book [:Tomcat] from comment #40)
> have to backout this changes because they block a m-i to m-c merge like
> 
> erging layout/reftests/image-element/reftest.list
> merging layout/reftests/svg/reftest.list
> warning: conflicts while merging editor/reftests/reftest.list! (edit, then
> use 'hg resolve --mark')
> warning: conflicts while merging layout/reftests/box-shadow/reftest.list!
> (edit, then use 'hg resolve --mark')
> warning: conflicts while merging layout/reftests/bugs/reftest.list! (edit,
> then use 'hg resolve --mark')
> warning: conflicts while merging layout/reftests/forms/select/reftest.list!
> (edit, then use 'hg resolve --mark')
> warning: conflicts while merging layout/reftests/image-element/reftest.list!
> (edit, then use 'hg resolve --mark')
> 1468 files updated, 8 files merged, 371 files removed, 5 files unresolved
> use 'hg resolve' to retry unresolved file merges or 'hg update -C .' to
> abandon

Le sigh, merged and fixed.

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5451e320b216
fix reftest typo. r=me

Comment 45

[Markus Stange [:mstange]]

(In reply to Lee Salzman [:lsalzman] from comment #39)
> Mason or Markus, do you have possibly ideas on why this might be the case?
> This is definitely somewhat of a mystery.

I'm surprised by this. Our reftests don't use a lot of -moz-appearance, so maybe that's how this bug evaded detection.
It should be possible to write a test for this particular bug.

Comment 46

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2aa2f6aa8053
https://hg.mozilla.org/mozilla-central/rev/cbb70e16727b
https://hg.mozilla.org/mozilla-central/rev/306f65d0a9eb
https://hg.mozilla.org/mozilla-central/rev/7dc2a529c1a9
https://hg.mozilla.org/mozilla-central/rev/eb088b241376
https://hg.mozilla.org/mozilla-central/rev/9d02032c3c3b
https://hg.mozilla.org/mozilla-central/rev/a97b1c46e56e
https://hg.mozilla.org/mozilla-central/rev/59b3cd42b4b5
https://hg.mozilla.org/mozilla-central/rev/6ba1d4e0178a
https://hg.mozilla.org/mozilla-central/rev/0ebe08d1aa92
https://hg.mozilla.org/mozilla-central/rev/e06077091df1
https://hg.mozilla.org/mozilla-central/rev/5451e320b216

Comment 47

[(not currently active) Ted Mielczarek]

Comment on attachment 8865116 [details] [diff] [review]
update moz.build for Skia m59

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

If this was part of the build I would be pickier, but as an import script it's fine. This might be useful to inform the work in bug 1336429, as well.

::: gfx/skia/generate_mozbuild.py
@@ +155,5 @@
> +          ('none', 'none', [':none'])]
> +
> +  opt_sources = {}
> +  for key, cpu, deps in cpus:
> +    subprocess.check_output('cd skia && bin/gn gen out/{0} --args=\'target_cpu="{1}"\''.format(key, cpu), shell=True)

FWIW, instead of having `cd skia` in there and doing string interpolation you can do:
subprocess.check_output(['bin/gin', 'gen', 'out/' + key, '--args=target_cpu="%s"' % cpu], cwd='skia')

Also if you're not using the output of the command, `subprocess.check_call` may be more appropriate.

Comment 48

[Robert Wood [:rwood]]

Awesome, looks like this patch has a performance win:

== Change summary for alert #6531 (as of May 10 2017 02:16 UTC) ==

Improvements:

 14%  tcanvasmark summary osx-10-10 opt e10s     6,369.58 -> 7,279.08

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6531

Comment 49

[Mason Chang [Inactive] [:mchang]]

Attachment: Ugly WIP Test

Ugly WIP Xul test that kinda mimics the top tab bar. I can only reproduce this with XUL elements. The weird thing is, I can repro this quite easily if I manually move my mouse between the elements. However, sending mouse events or clicking the events programmatically doesn't show the bug.

Comment 50

[Mason Chang [Inactive] [:mchang]]

Attachment: OS X UI Test to check for borrow/return CG Context

Minimal test case of the chrome UI tab bar. Since it's kinda random like the original bug, we have to iterate through the tabs a couple of times to guarantee that the bug surfaces.

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e527334fbcd
OS X XUL UI test to check for CG Borrow/Return CGContext r=lsalzman

Comment 52

[Mason Chang [Inactive] [:mchang]]

Inbound was open hoorah! Successful try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=955df9aabb0fffc5fdb0e677f1d5bdb36a101c32

Comment 53

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3e527334fbcd