Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
44573002:
Enabling validation code in serialization and adding serialization to fuzzer (Closed)
|
Created:
7 years, 1 month ago by sugoi1 Modified:
7 years, 1 month ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionEnabling validation code in serialization and adding serialization to fuzzer
BUG=
Committed: http://code.google.com/p/skia/source/detail?r=11968
Committed: http://code.google.com/p/skia/source/detail?r=11981
Patch Set 1 #
Total comments: 9
Patch Set 2 : Fixed memory leak in fuzzer #
Total comments: 2
Patch Set 3 : Removed extra ref from image filter #Patch Set 4 : Added missing static #Patch Set 5 : Fixed SkValidatingReadBuffer's constructor #
Messages
Total messages: 22 (0 generated)
https://codereview.chromium.org/44573002/diff/1/include/core/SkFlattenableSer... File include/core/SkFlattenableSerialization.h (right): https://codereview.chromium.org/44573002/diff/1/include/core/SkFlattenableSer... include/core/SkFlattenableSerialization.h:19: // Temporary fix for canary build Not sure I understand this. Is this changing all serialization to be temporarily validating? BTW the usual way to land these type of two-sided patches is to hide the old behaviour behind an #ifdef. Is that not possible here?
https://codereview.chromium.org/44573002/diff/1/include/core/SkFlattenableSer... File include/core/SkFlattenableSerialization.h (right): https://codereview.chromium.org/44573002/diff/1/include/core/SkFlattenableSer... include/core/SkFlattenableSerialization.h:19: // Temporary fix for canary build On 2013/10/25 16:13:11, Stephen White wrote: > Not sure I understand this. Is this changing all serialization to be temporarily > validating? > > BTW the usual way to land these type of two-sided patches is to hide the old > behaviour behind an #ifdef. Is that not possible here? I think this is just a name refinement. Sk{Serialize,Deserialize}Flattenable were introduced with validation in mind. https://codereview.chromium.org/44573002/diff/1/samplecode/SampleFilterFuzz.cpp File samplecode/SampleFilterFuzz.cpp (right): https://codereview.chromium.org/44573002/diff/1/samplecode/SampleFilterFuzz.c... samplecode/SampleFilterFuzz.cpp:266: SkImageFilter* filter = make_image_filter(false); Coming new at this, sorry if these are dumb questions. Where do this filter and data get deleted? https://codereview.chromium.org/44573002/diff/1/samplecode/SampleFilterFuzz.c... samplecode/SampleFilterFuzz.cpp:268: unsigned char* ptr = static_cast<unsigned char*>(const_cast<void*>(data->data())); Seems like you don't need the const_cast until you assign to p?
https://codereview.chromium.org/44573002/diff/1/include/core/SkFlattenableSer... File include/core/SkFlattenableSerialization.h (right): https://codereview.chromium.org/44573002/diff/1/include/core/SkFlattenableSer... include/core/SkFlattenableSerialization.h:19: // Temporary fix for canary build On 2013/10/25 16:17:31, mtklein wrote: > On 2013/10/25 16:13:11, Stephen White wrote: > > Not sure I understand this. Is this changing all serialization to be > temporarily > > validating? > > > > BTW the usual way to land these type of two-sided patches is to hide the old > > behaviour behind an #ifdef. Is that not possible here? > > I think this is just a name refinement. Sk{Serialize,Deserialize}Flattenable > were introduced with validation in mind. Ah, ok. Thanks. https://codereview.chromium.org/44573002/diff/1/samplecode/SampleFilterFuzz.cpp File samplecode/SampleFilterFuzz.cpp (right): https://codereview.chromium.org/44573002/diff/1/samplecode/SampleFilterFuzz.c... samplecode/SampleFilterFuzz.cpp:266: SkImageFilter* filter = make_image_filter(false); On 2013/10/25 16:17:31, mtklein wrote: > Coming new at this, sorry if these are dumb questions. Where do this filter and > data get deleted? Oooh, good catch. This probably leaks (Alexis can correct me if I'm wrong). There are two Skia-approved ways to handle this: - put the result of all new or factory functions into an SkAutoTUnref<Foo> at the callsite below (my preference) - call paint.setImageFilter(foo)->unref() to free the ref created by new/factoryfn (Grumble grumble need smart pointers grumble.)
https://codereview.chromium.org/44573002/diff/1/include/core/SkFlattenableSer... File include/core/SkFlattenableSerialization.h (right): https://codereview.chromium.org/44573002/diff/1/include/core/SkFlattenableSer... include/core/SkFlattenableSerialization.h:19: // Temporary fix for canary build On 2013/10/25 16:36:21, Stephen White wrote: > On 2013/10/25 16:17:31, mtklein wrote: > > On 2013/10/25 16:13:11, Stephen White wrote: > > > Not sure I understand this. Is this changing all serialization to be > > temporarily > > > validating? > > > > > > BTW the usual way to land these type of two-sided patches is to hide the old > > > behaviour behind an #ifdef. Is that not possible here? > > > > I think this is just a name refinement. Sk{Serialize,Deserialize}Flattenable > > were introduced with validation in mind. > > Ah, ok. Thanks. FYI, there's only a single user of these functions, and it's the ParamTraits class for SkImagefilter, and it's usage is currently hidden behind a flag in chrome. https://codereview.chromium.org/44573002/diff/1/samplecode/SampleFilterFuzz.cpp File samplecode/SampleFilterFuzz.cpp (right): https://codereview.chromium.org/44573002/diff/1/samplecode/SampleFilterFuzz.c... samplecode/SampleFilterFuzz.cpp:266: SkImageFilter* filter = make_image_filter(false); On 2013/10/25 16:36:21, Stephen White wrote: > On 2013/10/25 16:17:31, mtklein wrote: > > Coming new at this, sorry if these are dumb questions. Where do this filter > and > > data get deleted? > > Oooh, good catch. This probably leaks (Alexis can correct me if I'm wrong). > > There are two Skia-approved ways to handle this: > > - put the result of all new or factory functions into an SkAutoTUnref<Foo> at > the callsite below (my preference) > - call paint.setImageFilter(foo)->unref() to free the ref created by > new/factoryfn > > (Grumble grumble need smart pointers grumble.) I probably had an initial implementation that gave the ownership of these to another object, but I changed this code so many times for tests, that I probably removed all ownership of these at some point :) I added the SkAutoTUnref "smart" pointers. https://codereview.chromium.org/44573002/diff/1/samplecode/SampleFilterFuzz.c... samplecode/SampleFilterFuzz.cpp:268: unsigned char* ptr = static_cast<unsigned char*>(const_cast<void*>(data->data())); On 2013/10/25 16:17:31, mtklein wrote: > Seems like you don't need the const_cast until you assign to p? Done.
On 2013/10/25 17:15:04, sugoi1 wrote: > https://codereview.chromium.org/44573002/diff/1/include/core/SkFlattenableSer... > File include/core/SkFlattenableSerialization.h (right): > > https://codereview.chromium.org/44573002/diff/1/include/core/SkFlattenableSer... > include/core/SkFlattenableSerialization.h:19: // Temporary fix for canary build > On 2013/10/25 16:36:21, Stephen White wrote: > > On 2013/10/25 16:17:31, mtklein wrote: > > > On 2013/10/25 16:13:11, Stephen White wrote: > > > > Not sure I understand this. Is this changing all serialization to be > > > temporarily > > > > validating? > > > > > > > > BTW the usual way to land these type of two-sided patches is to hide the > old > > > > behaviour behind an #ifdef. Is that not possible here? > > > > > > I think this is just a name refinement. > Sk{Serialize,Deserialize}Flattenable > > > were introduced with validation in mind. > > > > Ah, ok. Thanks. > > FYI, there's only a single user of these functions, and it's the ParamTraits > class for SkImagefilter, and it's usage is currently hidden behind a flag in > chrome. > > https://codereview.chromium.org/44573002/diff/1/samplecode/SampleFilterFuzz.cpp > File samplecode/SampleFilterFuzz.cpp (right): > > https://codereview.chromium.org/44573002/diff/1/samplecode/SampleFilterFuzz.c... > samplecode/SampleFilterFuzz.cpp:266: SkImageFilter* filter = > make_image_filter(false); > On 2013/10/25 16:36:21, Stephen White wrote: > > On 2013/10/25 16:17:31, mtklein wrote: > > > Coming new at this, sorry if these are dumb questions. Where do this filter > > and > > > data get deleted? > > > > Oooh, good catch. This probably leaks (Alexis can correct me if I'm wrong). > > > > There are two Skia-approved ways to handle this: > > > > - put the result of all new or factory functions into an SkAutoTUnref<Foo> at > > the callsite below (my preference) > > - call paint.setImageFilter(foo)->unref() to free the ref created by > > new/factoryfn > > > > (Grumble grumble need smart pointers grumble.) > > I probably had an initial implementation that gave the ownership of these to > another object, but I changed this code so many times for tests, that I probably > removed all ownership of these at some point :) I added the SkAutoTUnref "smart" > pointers. > > https://codereview.chromium.org/44573002/diff/1/samplecode/SampleFilterFuzz.c... > samplecode/SampleFilterFuzz.cpp:268: unsigned char* ptr = static_cast<unsigned > char*>(const_cast<void*>(data->data())); > On 2013/10/25 16:17:31, mtklein wrote: > > Seems like you don't need the const_cast until you assign to p? > > Done. Not having a strong opinion on using an #ifdef vs. temporary #defines to do the two-tree handoff, this LGTM.
https://codereview.chromium.org/44573002/diff/100001/samplecode/SampleFilterF... File samplecode/SampleFilterFuzz.cpp (right): https://codereview.chromium.org/44573002/diff/100001/samplecode/SampleFilterF... samplecode/SampleFilterFuzz.cpp:278: SkFlattenable* flattenable = SkValidatingDeserializeFlattenable(ptr, len, This one looks like it leaks, too. I think it should be stored in an SkAutoTUnref<> in do_fuzz() below.
https://codereview.chromium.org/44573002/diff/100001/samplecode/SampleFilterF... File samplecode/SampleFilterFuzz.cpp (right): https://codereview.chromium.org/44573002/diff/100001/samplecode/SampleFilterF... samplecode/SampleFilterFuzz.cpp:278: SkFlattenable* flattenable = SkValidatingDeserializeFlattenable(ptr, len, On 2013/10/25 18:07:40, Stephen White wrote: > This one looks like it leaks, too. I think it should be stored in an > SkAutoTUnref<> in do_fuzz() below. Isn't the ownership transferred to SkPaint in SkPaint::setImageFilter ? Or do I have to write : paint.setImageFilter(make_serialized_image_filter())->unref();
On 2013/10/25 18:18:25, sugoi1 wrote: > https://codereview.chromium.org/44573002/diff/100001/samplecode/SampleFilterF... > File samplecode/SampleFilterFuzz.cpp (right): > > https://codereview.chromium.org/44573002/diff/100001/samplecode/SampleFilterF... > samplecode/SampleFilterFuzz.cpp:278: SkFlattenable* flattenable = > SkValidatingDeserializeFlattenable(ptr, len, > On 2013/10/25 18:07:40, Stephen White wrote: > > This one looks like it leaks, too. I think it should be stored in an > > SkAutoTUnref<> in do_fuzz() below. > > Isn't the ownership transferred to SkPaint in SkPaint::setImageFilter ? > Or do I have to write : > paint.setImageFilter(make_serialized_image_filter())->unref(); The latter (or use an SkAutoTUnref<> when you call make_foo()).
LGTM
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/44573002/160001
Presubmit check for 44573002-160001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Since the CL is editing public API, you must have an LGTM from one of: ('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org', 'bsalomon@google.com')
On 2013/10/25 20:00:57, I haz the power (commit-bot) wrote: > Presubmit check for 44573002-160001 failed and returned exit status 1. > > Running presubmit commit checks ... > > ** Presubmit ERRORS ** > Since the CL is editing public API, you must have an LGTM from one of: > ('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org', > 'bsalomon@google.com') lgtm
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/44573002/160001
Retried try job too often on Build-Mac10.7-Clang-x86-Release-Trybot for step(s) BuildEverything http://skiabot-master.pogerlabs.com:10117/buildstatus?builder=Build-Mac10.7-C...
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/44573002/310001
Message was sent while issue was closed.
Change committed as 11968
Message was sent while issue was closed.
This was reverted in 11974 due to a crash in Chromium's AllQuads unit test. Here is the call stack: out/Debug/content_unittests --gtest_filter=CCMessagesTest.AllQuads Note: Google Test filter = CCMessagesTest.AllQuads [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from CCMessagesTest [ RUN ] CCMessagesTest.AllQuads Received signal 11 SEGV_MAPERR 000000000010 [0x000001bed204] base::debug::StackTrace::StackTrace() [0x000001becb0b] base::debug::(anonymous namespace)::StackDumpSignalHandler() [0x7f01d5b2dcb0] <unknown> [0x000000928ed2] SkImageFilter::countInputs() [0x0000009225fd] content::(anonymous namespace)::CCMessagesTest::Compare() [0x0000009217cd] content::(anonymous namespace)::CCMessagesTest::Compare() [0x000000927374] content::(anonymous namespace)::CCMessagesTest_AllQuads_Test::TestBody() [0x0000013a88f1] testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x0000013a6015] testing::internal::HandleExceptionsInMethodIfSupported<>() [0x00000139c0ed] testing::Test::Run() [0x00000139c6ca] testing::TestInfo::Run() [0x00000139cbd8] testing::TestCase::Run() [0x0000013a11d8] testing::internal::UnitTestImpl::RunAllTests() [0x0000013a96c3] testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x0000013a67a9] testing::internal::HandleExceptionsInMethodIfSupported<>() [0x0000013a0323] testing::UnitTest::Run() [0x00000135cc54] base::TestSuite::Run() [0x0000012ec22e] content::UnitTestTestSuite::Run() [0x000000a48ed6] base::internal::RunnableAdapter<>::Run() [0x000000a48e5b] base::internal::InvokeHelper<>::MakeItSo() [0x000000a48e07] base::internal::Invoker<>::Run() [0x00000100c629] base::Callback<>::Run() [0x00000135723d] base::LaunchUnitTests() [0x000000a48bb9] main [0x7f01d48f276d] __libc_start_main [0x00000042a319] <unknown> r8: 00007fff63772ee4 r9: 0101010101010101 r10: 0000000000000000 r11: 00007f01d495ce16 r12: 0000000000000007 r13: 0000000000000001 r14: 0000000000000001 r15: 0000000000000001 di: 0000000000000000 si: 00007f01d1c83330 bp: 00007fff63772d10 bx: 00007f01d1c832a0 dx: 00007fff63772e80 ax: 0000000000000000 cx: 00007fff63772d0c sp: 00007fff63772d10 ip: 0000000000928ed2 efl: 0000000000010246 cgf: 0000000000000033 erf: 0000000000000004 trp: 000000000000000e msk: 0000000000000000 cr2: 0000000000000010
Message was sent while issue was closed.
I'm on it.
Message was sent while issue was closed.
Fixed it. It was just a silly mistake. Inside SkValidatingReadBuffer's constructor, setMemory was called BEFORE fError was set to false, so setMemory would fail any time fError was true at this point (which may or may not happen, since this was uninitialized memory).
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/44573002/380001
Message was sent while issue was closed.
Change committed as 11981 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
