Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
948163002:
Indexed PNG decoding: Ensure color table is large enough that the bit depth of the image will not a… (Closed)
|
Created:
5 years, 10 months ago by David Lattimore Modified:
5 years, 9 months ago Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionIndexed PNG decoding: Ensure color table is large enough that the bit depth of the image will not allow reads beyond its end.
BUG=skia:3440
R=rmistry@google.com, scroggo@google.com
Committed: https://skia.googlesource.com/skia/+/493c1ce1cd406ef28683203146274154783452ce
Committed: https://skia.googlesource.com/skia/+/78acf96a67db1fe64a89fa7207195d1a79efc0c3
Patch Set 1 #
Total comments: 12
Patch Set 2 : Moved input argument before output arguments + various changes to test. #Patch Set 3 : Disable test on platforms that don't use libpng #
Total comments: 2
Patch Set 4 : Fixed comment in test #
Total comments: 5
Patch Set 5 : Fix indentation in test. Change test to only run on Unix. #Patch Set 6 : Made test run on all platforms #Patch Set 7 : Fixed argument order in some conditional Android code. #
Messages
Total messages: 37 (5 generated)
dml@google.com changed reviewers: + scroggo@google.com
Here's another change that I didn't mail. Thanks for the tip.
Besides the below comments the code looks good. https://codereview.chromium.org/948163002/diff/1/src/images/SkImageDecoder_li... File src/images/SkImageDecoder_libpng.cpp (right): https://codereview.chromium.org/948163002/diff/1/src/images/SkImageDecoder_li... src/images/SkImageDecoder_libpng.cpp:102: SkColorTable **colorTablep, int bitDepth); nit: I would prefer to have this new input parameter before all these output parameters. https://codereview.chromium.org/948163002/diff/1/tests/IndexedPngOverflowTest... File tests/IndexedPngOverflowTest.cpp (right): https://codereview.chromium.org/948163002/diff/1/tests/IndexedPngOverflowTest... tests/IndexedPngOverflowTest.cpp:2: * Copyright 2011 Google Inc. 2015* https://codereview.chromium.org/948163002/diff/1/tests/IndexedPngOverflowTest... tests/IndexedPngOverflowTest.cpp:19: unsigned char a_png[] = { gPng* https://codereview.chromium.org/948163002/diff/1/tests/IndexedPngOverflowTest... tests/IndexedPngOverflowTest.cpp:43: .makeColorType(kBGRA_8888_SkColorType)); Is there a reason you want BGRA instead of N32 (which is either BGRA or RGBA)? https://codereview.chromium.org/948163002/diff/1/tests/IndexedPngOverflowTest... tests/IndexedPngOverflowTest.cpp:46: SkCanvas* canvas = new SkCanvas(device); This is the old way of creating an SkCanvas. Here's the new process: SkAutoTUnref<SkSurface> surface(SkSurface::NewRaster(SkImageInfo::MakeN32Premul(20,1))); SkCanvas* canvas = surface->getCanvas(); https://codereview.chromium.org/948163002/diff/1/tests/IndexedPngOverflowTest... tests/IndexedPngOverflowTest.cpp:48: canvas->drawBitmapRect(image, destRect, nullptr); nit: Most of our code uses NULL. (The only exception I found that uses nullptr is in Android only code.)
After syncing, the test was failing. It seems that indexed PNGs with a size-0 palette are no longer considered valid (fair enough). So I created a new PNG with a palette size of 2 and put that in. I confirmed that the buffer overread still happened without my fix. https://codereview.chromium.org/948163002/diff/1/src/images/SkImageDecoder_li... File src/images/SkImageDecoder_libpng.cpp (right): https://codereview.chromium.org/948163002/diff/1/src/images/SkImageDecoder_li... src/images/SkImageDecoder_libpng.cpp:102: SkColorTable **colorTablep, int bitDepth); On 2015/03/05 23:23:30, scroggo wrote: > nit: I would prefer to have this new input parameter before all these output > parameters. Done. https://codereview.chromium.org/948163002/diff/1/tests/IndexedPngOverflowTest... File tests/IndexedPngOverflowTest.cpp (right): https://codereview.chromium.org/948163002/diff/1/tests/IndexedPngOverflowTest... tests/IndexedPngOverflowTest.cpp:2: * Copyright 2011 Google Inc. On 2015/03/05 23:23:30, scroggo wrote: > 2015* Done. https://codereview.chromium.org/948163002/diff/1/tests/IndexedPngOverflowTest... tests/IndexedPngOverflowTest.cpp:19: unsigned char a_png[] = { On 2015/03/05 23:23:30, scroggo wrote: > gPng* Done. https://codereview.chromium.org/948163002/diff/1/tests/IndexedPngOverflowTest... tests/IndexedPngOverflowTest.cpp:43: .makeColorType(kBGRA_8888_SkColorType)); On 2015/03/05 23:23:30, scroggo wrote: > Is there a reason you want BGRA instead of N32 (which is either BGRA or RGBA)? Nope. I just based this off some other code I had where I did have a good reason, but I don't here. Removed. https://codereview.chromium.org/948163002/diff/1/tests/IndexedPngOverflowTest... tests/IndexedPngOverflowTest.cpp:46: SkCanvas* canvas = new SkCanvas(device); On 2015/03/05 23:23:30, scroggo wrote: > This is the old way of creating an SkCanvas. Here's the new process: > > SkAutoTUnref<SkSurface> > surface(SkSurface::NewRaster(SkImageInfo::MakeN32Premul(20,1))); > SkCanvas* canvas = surface->getCanvas(); Done. https://codereview.chromium.org/948163002/diff/1/tests/IndexedPngOverflowTest... tests/IndexedPngOverflowTest.cpp:48: canvas->drawBitmapRect(image, destRect, nullptr); On 2015/03/05 23:23:30, scroggo wrote: > nit: Most of our code uses NULL. (The only exception I found that uses nullptr > is in Android only code.) Done.
On 2015/03/06 00:15:26, David Lattimore wrote: > After syncing, the test was failing. It seems that indexed PNGs with a size-0 > palette are no longer considered valid (fair enough). So I created a new PNG > with a palette size of 2 and put that in. I confirmed that the buffer overread > still happened without my fix. I'm guessing that's because our version of PNG changed? LGTM
Ravi, I seem to be unable to commit, even though it has an LGTM (from me). The checkbox is grayed out in the old UI, and the button doesn't seem to do anything with the new one. Any idea what is going on?
scroggo@google.com changed reviewers: + rmistry@google.com
On 2015/03/06 19:58:24, scroggo wrote: > Ravi, > > I seem to be unable to commit, even though it has an LGTM (from me). The > checkbox is grayed out in the old UI, and the button doesn't seem to do anything > with the new one. Any idea what is going on?
On 2015/03/06 19:58:24, scroggo wrote: > Ravi, > > I seem to be unable to commit, even though it has an LGTM (from me). The > checkbox is grayed out in the old UI, and the button doesn't seem to do anything > with the new one. Any idea what is going on? It is because the CL is marked as "Private/Protected. Only viewable by @chromium and @google accounts.". Those CLs cannot be CQ'ed into a public repo. To land it should be either unprotected or git cl landed.
On 2015/03/06 20:50:56, rmistry wrote: > On 2015/03/06 19:58:24, scroggo wrote: > > Ravi, > > > > I seem to be unable to commit, even though it has an LGTM (from me). The > > checkbox is grayed out in the old UI, and the button doesn't seem to do > anything > > with the new one. Any idea what is going on? > > It is because the CL is marked as "Private/Protected. Only viewable by @chromium > and @google accounts.". Those CLs cannot be CQ'ed into a public repo. To land it > should be either unprotected or git cl landed. I marked it private because this is a potentially exploitable security issue (allows up to 1KB blocks of heap to be read). I've just uploaded a new patch that disables the test on platforms that don't use libpng.
On 2015/03/06 20:50:56, rmistry wrote: > It is because the CL is marked as "Private/Protected. Only viewable by @chromium > and @google accounts.". Those CLs cannot be CQ'ed into a public repo. To land it > should be either unprotected or git cl landed. Thanks for the info, Ravi. I'll "git cl land" it once the tree is green. (I'm guessing you cannot do it, dml@, since you're not a member of the Skia team, but honestly I don't know how those access rights work.) On 2015/03/09 00:50:38, David Lattimore wrote: > I've just uploaded a new patch that disables the test on platforms that don't > use libpng. LGTM, besides a nit on your comment https://codereview.chromium.org/948163002/diff/40001/tests/IndexedPngOverflow... File tests/IndexedPngOverflowTest.cpp (right): https://codereview.chromium.org/948163002/diff/40001/tests/IndexedPngOverflow... tests/IndexedPngOverflowTest.cpp:8: // The PNG decoder is not available on all platforms. Nit: this comment sounds like we cannot decode PNGs on all platforms, whereas really SkPNGImageDecoder is not available on all platforms.
On 2015/03/11 13:34:09, scroggo wrote: > On 2015/03/06 20:50:56, rmistry wrote: > > It is because the CL is marked as "Private/Protected. Only viewable by > @chromium > > and @google accounts.". Those CLs cannot be CQ'ed into a public repo. To land > it > > should be either unprotected or git cl landed. > > Thanks for the info, Ravi. I'll "git cl land" it once the tree is green. (I'm > guessing you cannot do it, dml@, since you're not a member of the Skia team, but > honestly I don't know how those access rights work.) dml@ should be able to "git cl land" it once you give an LGTM. > > On 2015/03/09 00:50:38, David Lattimore wrote: > > I've just uploaded a new patch that disables the test on platforms that don't > > use libpng. > > LGTM, besides a nit on your comment > > https://codereview.chromium.org/948163002/diff/40001/tests/IndexedPngOverflow... > File tests/IndexedPngOverflowTest.cpp (right): > > https://codereview.chromium.org/948163002/diff/40001/tests/IndexedPngOverflow... > tests/IndexedPngOverflowTest.cpp:8: // The PNG decoder is not available on all > platforms. > Nit: this comment sounds like we cannot decode PNGs on all platforms, whereas > really SkPNGImageDecoder is not available on all platforms.
not LGTM. It looks like SK_BUILD_FOR_WIN is not a good check. https://codereview.chromium.org/948163002/diff/60001/tests/IndexedPngOverflow... File tests/IndexedPngOverflowTest.cpp (right): https://codereview.chromium.org/948163002/diff/60001/tests/IndexedPngOverflow... tests/IndexedPngOverflowTest.cpp:10: !defined(SK_BUILD_FOR_WIN) && \ It turns out we cannot depend on this build flag. See https://codereview.chromium.org/1002583002/ FWIW, it might run successfuly as is on Windows. https://codereview.chromium.org/948163002/diff/60001/tests/IndexedPngOverflow... tests/IndexedPngOverflowTest.cpp:38: SkBitmap image; nit: Skia uses 4 space indent.
Thanks for the fix on my other CL. https://codereview.chromium.org/948163002/diff/40001/tests/IndexedPngOverflow... File tests/IndexedPngOverflowTest.cpp (right): https://codereview.chromium.org/948163002/diff/40001/tests/IndexedPngOverflow... tests/IndexedPngOverflowTest.cpp:8: // The PNG decoder is not available on all platforms. On 2015/03/11 13:34:09, scroggo wrote: > Nit: this comment sounds like we cannot decode PNGs on all platforms, whereas > really SkPNGImageDecoder is not available on all platforms. Done. https://codereview.chromium.org/948163002/diff/60001/tests/IndexedPngOverflow... File tests/IndexedPngOverflowTest.cpp (right): https://codereview.chromium.org/948163002/diff/60001/tests/IndexedPngOverflow... tests/IndexedPngOverflowTest.cpp:10: !defined(SK_BUILD_FOR_WIN) && \ On 2015/03/11 21:01:32, scroggo wrote: > It turns out we cannot depend on this build flag. See > https://codereview.chromium.org/1002583002/ > > FWIW, it might run successfuly as is on Windows. I've changed it so that it only runs on UNIX. Does that seem reasonable? https://codereview.chromium.org/948163002/diff/60001/tests/IndexedPngOverflow... tests/IndexedPngOverflowTest.cpp:38: SkBitmap image; On 2015/03/11 21:01:32, scroggo wrote: > nit: Skia uses 4 space indent. Done.
scroggo@google.com changed reviewers: + mtklein@google.com
Mike, this test is only interesting on ASAN. Does it make sense to only build the test if THREAD_SANITIZER is defined? https://codereview.chromium.org/948163002/diff/60001/tests/IndexedPngOverflow... File tests/IndexedPngOverflowTest.cpp (right): https://codereview.chromium.org/948163002/diff/60001/tests/IndexedPngOverflow... tests/IndexedPngOverflowTest.cpp:10: !defined(SK_BUILD_FOR_WIN) && \ On 2015/03/11 21:47:21, David Lattimore wrote: > On 2015/03/11 21:01:32, scroggo wrote: > > It turns out we cannot depend on this build flag. See > > https://codereview.chromium.org/1002583002/ > > > > FWIW, it might run successfuly as is on Windows. > > I've changed it so that it only runs on UNIX. Does that seem reasonable? Probably... Android is the main place we care, and I also want to add SkCodec (the replacement to SkImageDecoder) to this test, and it will be the same on all platforms (unlike SkImageDecoder). (That will require updating SkPngCodec in SkCodec_libpng.cpp, a file I recently wrote, based off of the one you're updating. I think it's fine to leave that to a separate CL.) That said, ASAN only runs on on UNIX. We could also be more picky, and only run with ASAN. I don't know if we have define for that, but it looks like we can check #ifdef THREAD_SANITIZER. For consistency (see skbug.com/3362), we'll need to include SkTypes first. So the top of this file should say #include "SkTypes.h" #ifdef THREAD_SANITIZER
On 2015/03/12 14:13:01, scroggo wrote: > Mike, this test is only interesting on ASAN. Does it make sense to only build > the test if THREAD_SANITIZER is defined? > > https://codereview.chromium.org/948163002/diff/60001/tests/IndexedPngOverflow... > File tests/IndexedPngOverflowTest.cpp (right): > > https://codereview.chromium.org/948163002/diff/60001/tests/IndexedPngOverflow... > tests/IndexedPngOverflowTest.cpp:10: !defined(SK_BUILD_FOR_WIN) && \ > On 2015/03/11 21:47:21, David Lattimore wrote: > > On 2015/03/11 21:01:32, scroggo wrote: > > > It turns out we cannot depend on this build flag. See > > > https://codereview.chromium.org/1002583002/ > > > > > > FWIW, it might run successfuly as is on Windows. > > > > I've changed it so that it only runs on UNIX. Does that seem reasonable? > > Probably... Android is the main place we care, and I also want to add SkCodec > (the replacement to SkImageDecoder) to this test, and it will be the same on all > platforms (unlike SkImageDecoder). (That will require updating SkPngCodec in > SkCodec_libpng.cpp, a file I recently wrote, based off of the one you're > updating. I think it's fine to leave that to a separate CL.) That said, ASAN > only runs on on UNIX. We could also be more picky, and only run with ASAN. I > don't know if we have define for that, but it looks like we can check #ifdef > THREAD_SANITIZER. > > For consistency (see skbug.com/3362), we'll need to include SkTypes first. So > the top of this file should say > > #include "SkTypes.h" > #ifdef THREAD_SANITIZER Not really. THREAD_SANITIZER is not set for ASAN runs, but you could define ADDRESS_SANITIZER and check for that if you like, adding something like this in common_conditions.gyp: [ 'skia_sanitizer == "address"', { 'defines': [ 'ADDRESS_SANITIZER' ], }], That said, I'd prefer you not, and just run it everywhere instead. If it absolutely cannot run under any configuration except ASAN, that's weird, but then it might make sense to check. What's wrong with checking defined(SK_BUILD_FOR_WIN)? That should be the right thing to check that we're building for Windows, which is not quite the same as defined(_MSC_VER) anymore.
On 2015/03/12 14:30:42, mtklein wrote: > What's wrong with checking defined(SK_BUILD_FOR_WIN)? That should be the right > thing to check that we're building for Windows, which is not quite the same as > defined(_MSC_VER) anymore. It doesn't work. We tried that in https://codereview.chromium.org/951663002/. Ben tells me it's not a good way to check, and recommended deciding whether to build the file in the gyp file (I believe based on skia_os not in ["mac", "win", "ios"]). > That said, I'd prefer you not, and just run it everywhere instead. Sounds good to me. David, please remove the #ifdef, and then I think it's good to go.
On 2015/03/12 14:42:49, scroggo wrote: > On 2015/03/12 14:30:42, mtklein wrote: > > What's wrong with checking defined(SK_BUILD_FOR_WIN)? That should be the > right > > thing to check that we're building for Windows, which is not quite the same as > > defined(_MSC_VER) anymore. > > It doesn't work. We tried that in https://codereview.chromium.org/951663002/. > Ben tells me it's not a good way to check, and recommended deciding whether to > build the file in the gyp file (I believe based on skia_os not in ["mac", "win", > "ios"]). I think I get it. This is the same problem as your Android builds. You need to include SkTypes before checking #ifdef SK_BUILD_FOR_WIN. SK_BUILD_FOR_WIN works fine.
On 2015/03/12 14:54:35, mtklein wrote: > On 2015/03/12 14:42:49, scroggo wrote: > > On 2015/03/12 14:30:42, mtklein wrote: > > > What's wrong with checking defined(SK_BUILD_FOR_WIN)? That should be the > > right > > > thing to check that we're building for Windows, which is not quite the same > as > > > defined(_MSC_VER) anymore. > > > > It doesn't work. We tried that in https://codereview.chromium.org/951663002/. > > Ben tells me it's not a good way to check, and recommended deciding whether to > > build the file in the gyp file (I believe based on skia_os not in ["mac", > "win", > > "ios"]). > > I think I get it. This is the same problem as your Android builds. You need to > include SkTypes before checking #ifdef SK_BUILD_FOR_WIN. SK_BUILD_FOR_WIN works > fine. Oh the irony! I still think we should go ahead and build the test everywhere.
On 2015/03/12 14:57:51, scroggo wrote: > On 2015/03/12 14:54:35, mtklein wrote: > > On 2015/03/12 14:42:49, scroggo wrote: > > > On 2015/03/12 14:30:42, mtklein wrote: > > > > What's wrong with checking defined(SK_BUILD_FOR_WIN)? That should be the > > > right > > > > thing to check that we're building for Windows, which is not quite the > same > > as > > > > defined(_MSC_VER) anymore. > > > > > > It doesn't work. We tried that in > https://codereview.chromium.org/951663002/. > > > Ben tells me it's not a good way to check, and recommended deciding whether > to > > > build the file in the gyp file (I believe based on skia_os not in ["mac", > > "win", > > > "ios"]). > > > > I think I get it. This is the same problem as your Android builds. You need > to > > include SkTypes before checking #ifdef SK_BUILD_FOR_WIN. SK_BUILD_FOR_WIN > works > > fine. > > Oh the irony! I still think we should go ahead and build the test everywhere. OK, I've removed the #ifdef. Probably the main risk of failure on non-Unix platforms would be if one of the platforms considered the PNG to be invalid and refused to decode it. That said, I was thinking about my other CL - it seems like the PNG decoders on the other platforms are if anything more lenient, since they were accepting the corrupted PNG where SkPNGImageDecoder_libpng wasn't. So hopefully that trend applies to this PNG as well and all platforms accept it. If there's a way to run this test on a all (or a few) different platforms before we submit, that could be good. I don't like breaking things.
On 2015/03/12 20:53:03, David Lattimore wrote: > On 2015/03/12 14:57:51, scroggo wrote: > > On 2015/03/12 14:54:35, mtklein wrote: > > > On 2015/03/12 14:42:49, scroggo wrote: > > > > On 2015/03/12 14:30:42, mtklein wrote: > > > > > What's wrong with checking defined(SK_BUILD_FOR_WIN)? That should be > the > > > > right > > > > > thing to check that we're building for Windows, which is not quite the > > same > > > as > > > > > defined(_MSC_VER) anymore. > > > > > > > > It doesn't work. We tried that in > > https://codereview.chromium.org/951663002/. > > > > Ben tells me it's not a good way to check, and recommended deciding > whether > > to > > > > build the file in the gyp file (I believe based on skia_os not in ["mac", > > > "win", > > > > "ios"]). > > > > > > I think I get it. This is the same problem as your Android builds. You > need > > to > > > include SkTypes before checking #ifdef SK_BUILD_FOR_WIN. SK_BUILD_FOR_WIN > > works > > > fine. > > > > Oh the irony! I still think we should go ahead and build the test everywhere. > > OK, I've removed the #ifdef. Probably the main risk of failure on non-Unix > platforms would be if one of the platforms considered the PNG to be invalid and > refused to decode it. That said, I was thinking about my other CL - it seems > like the PNG decoders on the other platforms are if anything more lenient, since > they were accepting the corrupted PNG where SkPNGImageDecoder_libpng wasn't. So > hopefully that trend applies to this PNG as well and all platforms accept it. If > there's a way to run this test on a all (or a few) different platforms before we > submit, that could be good. I don't like breaking things. If I understand correctly, because this is a private/protected review, you cannot run trybots. So we're stuck with manually testing. I can test on my Mac at home (which I think should cover iOS as well, since I believe it uses SkImageDecoder_CG), but I don't have a windows machine set up for building Skia....
On 2015/03/12 20:56:14, scroggo wrote: > On 2015/03/12 20:53:03, David Lattimore wrote: > > On 2015/03/12 14:57:51, scroggo wrote: > > > On 2015/03/12 14:54:35, mtklein wrote: > > > > On 2015/03/12 14:42:49, scroggo wrote: > > > > > On 2015/03/12 14:30:42, mtklein wrote: > > > > > > What's wrong with checking defined(SK_BUILD_FOR_WIN)? That should be > > the > > > > > right > > > > > > thing to check that we're building for Windows, which is not quite the > > > same > > > > as > > > > > > defined(_MSC_VER) anymore. > > > > > > > > > > It doesn't work. We tried that in > > > https://codereview.chromium.org/951663002/. > > > > > Ben tells me it's not a good way to check, and recommended deciding > > whether > > > to > > > > > build the file in the gyp file (I believe based on skia_os not in > ["mac", > > > > "win", > > > > > "ios"]). > > > > > > > > I think I get it. This is the same problem as your Android builds. You > > need > > > to > > > > include SkTypes before checking #ifdef SK_BUILD_FOR_WIN. SK_BUILD_FOR_WIN > > > works > > > > fine. > > > > > > Oh the irony! I still think we should go ahead and build the test > everywhere. > > > > OK, I've removed the #ifdef. Probably the main risk of failure on non-Unix > > platforms would be if one of the platforms considered the PNG to be invalid > and > > refused to decode it. That said, I was thinking about my other CL - it seems > > like the PNG decoders on the other platforms are if anything more lenient, > since > > they were accepting the corrupted PNG where SkPNGImageDecoder_libpng wasn't. > So > > hopefully that trend applies to this PNG as well and all platforms accept it. > If > > there's a way to run this test on a all (or a few) different platforms before > we > > submit, that could be good. I don't like breaking things. > > If I understand correctly, because this is a private/protected review, you > cannot run trybots. So we're stuck with manually testing. I can test on my Mac > at home (which I think should cover iOS as well, since I believe it uses > SkImageDecoder_CG), but I don't have a windows machine set up for building > Skia.... LGTM
On 2015/03/12 20:56:14, scroggo wrote: > If I understand correctly, because this is a private/protected review, you > cannot run trybots. So we're stuck with manually testing. I can test on my Mac > at home (which I think should cover iOS as well, since I believe it uses > SkImageDecoder_CG), but I don't have a windows machine set up for building > Skia.... Test passes on my mac.
On 2015/03/13 14:07:36, scroggo wrote: > Test passes on my mac. Thanks for trying that out. I just tried to land the CL, but it seems I'm not allowed. It said "fatal: remote error: Git access forbidden"
On 2015/03/15 23:01:10, David Lattimore wrote: > On 2015/03/13 14:07:36, scroggo wrote: > > Test passes on my mac. > > Thanks for trying that out. > > I just tried to land the CL, but it seems I'm not allowed. It said "fatal: > remote error: Git access forbidden" What is the command you used to land the CL?
On 2015/03/15 23:11:10, rmistry wrote: > On 2015/03/15 23:01:10, David Lattimore wrote: > > On 2015/03/13 14:07:36, scroggo wrote: > > > Test passes on my mac. > > > > Thanks for trying that out. > > > > I just tried to land the CL, but it seems I'm not allowed. It said "fatal: > > remote error: Git access forbidden" > > What is the command you used to land the CL? "git cl land". Before running that, I clicked "Generate password" at skia.googlesource.com then pasted that into my shell.
On 2015/03/15 23:34:48, David Lattimore wrote: > On 2015/03/15 23:11:10, rmistry wrote: > > On 2015/03/15 23:01:10, David Lattimore wrote: > > > On 2015/03/13 14:07:36, scroggo wrote: > > > > Test passes on my mac. > > > > > > Thanks for trying that out. > > > > > > I just tried to land the CL, but it seems I'm not allowed. It said "fatal: > > > remote error: Git access forbidden" > > > > What is the command you used to land the CL? > > "git cl land". Before running that, I clicked "Generate password" at > http://skia.googlesource.com then pasted that into my shell. I took another look at depot_tools and I was mistaken, you cannot "git cl land" unless you have the Skia team's ACLs, apologies for the confusion. Leon will have to land it for you.
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as 493c1ce1cd406ef28683203146274154783452ce (presubmit successful).
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/997183007/ by scroggo@google.com. The reason for reverting is: Breaking compile bots.
Message was sent while issue was closed.
On 2015/03/17 12:21:25, scroggo wrote: > A revert of this CL (patchset #6 id:100001) has been created in > https://codereview.chromium.org/997183007/ by mailto:scroggo@google.com. > > The reason for reverting is: Breaking compile bots. That one didn't work. Landed a revert with https://codereview.chromium.org/1014553003 Example error that caused the breakage is here: http://build.chromium.org/p/client.skia.compile/builders/Build-Mac10.8-Clang-...
Message was sent while issue was closed.
Sorry about the breakage. I've uploaded a fix. It should now be possible to submit via the commit bot.
On 2015/03/17 23:32:34, David Lattimore wrote: > Sorry about the breakage. I've uploaded a fix. It should now be possible to > submit via the commit bot. I've reopened. LGTM. I'll put it in the CQ
The CQ bit was checked by scroggo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rmistry@google.com Link to the patchset: https://codereview.chromium.org/948163002/#ps120001 (title: "Fixed argument order in some conditional Android code.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/948163002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://skia.googlesource.com/skia/+/78acf96a67db1fe64a89fa7207195d1a79efc0c3 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
