Bugfix that cause skpdiff to segfault.

For certain input skpdiff segfaulted. This prevents the segfault and should fix the failed test in housekeeping.

BUG=skia:2902
Committed: https://skia.googlesource.com/skia/+/7260d7292bde966f038b8e166f3fe41ddd8f2099

[stephana, 2014-09-05 17:01:08]

stephana@google.com changed reviewers:
+ mtklein@google.com

[stephana, 2014-09-05 17:03:21]

https://codereview.chromium.org/546753003/diff/1/tools/skpdiff/SkDiffContext.cpp
File tools/skpdiff/SkDiffContext.cpp (right):

https://codereview.chromium.org/546753003/diff/1/tools/skpdiff/SkDiffContext....
tools/skpdiff/SkDiffContext.cpp:292: tg.wait();
This line should not be necessary, because wait() is called in the destructor of
SkTaskGroup and therefore should be called when 'tg' goes out of scope.

[mtklein, 2014-09-05 17:18:51]

https://codereview.chromium.org/546753003/diff/1/tools/skpdiff/SkDiffContext.cpp
File tools/skpdiff/SkDiffContext.cpp (right):

https://codereview.chromium.org/546753003/diff/1/tools/skpdiff/SkDiffContext....
tools/skpdiff/SkDiffContext.cpp:292: tg.wait();
On 2014/09/05 17:03:20, stephana wrote:
> This line should not be necessary, because wait() is called in the destructor
of
> SkTaskGroup and therefore should be called when 'tg' goes out of scope.

I agree.  So why does it fix the crash?

[stephana, 2014-09-05 18:38:16]

On 2014/09/05 17:18:51, mtklein wrote:
>
https://codereview.chromium.org/546753003/diff/1/tools/skpdiff/SkDiffContext.cpp
> File tools/skpdiff/SkDiffContext.cpp (right):
> 
>
https://codereview.chromium.org/546753003/diff/1/tools/skpdiff/SkDiffContext....
> tools/skpdiff/SkDiffContext.cpp:292: tg.wait();
> On 2014/09/05 17:03:20, stephana wrote:
> > This line should not be necessary, because wait() is called in the
destructor
> of
> > SkTaskGroup and therefore should be called when 'tg' goes out of scope.
> 
> I agree.  So why does it fix the crash?

I don't know. Could be a strange fluke with the data, but I think it happened
with a number inputs, not just the one I tested. 
I did not trace through it without that line, but I saw in gdb that it crashed
in the destructor of SkTaskGroup, so I added the explicit wait() on a hunch.

[mtklein, 2014-09-05 18:46:43]

https://codereview.chromium.org/546753003/diff/20001/tools/skpdiff/SkDiffCont...
File tools/skpdiff/SkDiffContext.cpp (right):

https://codereview.chromium.org/546753003/diff/20001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.cpp:287: 
Ooh, I see.  Try moving "SkTaskGroup tg;" down to just before the for loop, then
remove the call to tg.wait()?

runnableDiffs goes out of scope before tg, so it's deleted before we call
wait().

[stephana, 2014-09-05 19:26:36]

On 2014/09/05 18:46:43, mtklein wrote:
>
https://codereview.chromium.org/546753003/diff/20001/tools/skpdiff/SkDiffCont...
> File tools/skpdiff/SkDiffContext.cpp (right):
> 
>
https://codereview.chromium.org/546753003/diff/20001/tools/skpdiff/SkDiffCont...
> tools/skpdiff/SkDiffContext.cpp:287: 
> Ooh, I see.  Try moving "SkTaskGroup tg;" down to just before the for loop,
then
> remove the call to tg.wait()?
> 
> runnableDiffs goes out of scope before tg, so it's deleted before we call
> wait().

I agree, that should fix the problem and I'll try it to see. 
IMHO though it's desirable to call wait() explicitly rather than relying on the
destructor. 
Maybe that's not a problem, but it would be nice if we could enforce that
somehow, i.e. the destructor issue a warning before the call to wait.

[stephana, 2014-09-05 19:51:11]

stephana@google.com changed reviewers:
+ rmistry@google.com

[stephana, 2014-09-05 19:52:25]

I think this will turn the Housekeeping tests green. 

Ravi, could you review the python stuff ?

[rmistry, 2014-09-05 19:53:38]

LGTM

[mtklein, 2014-09-05 20:01:04]

On 2014/09/05 19:26:36, stephana wrote:
> On 2014/09/05 18:46:43, mtklein wrote:
> >
>
https://codereview.chromium.org/546753003/diff/20001/tools/skpdiff/SkDiffCont...
> > File tools/skpdiff/SkDiffContext.cpp (right):
> > 
> >
>
https://codereview.chromium.org/546753003/diff/20001/tools/skpdiff/SkDiffCont...
> > tools/skpdiff/SkDiffContext.cpp:287: 
> > Ooh, I see.  Try moving "SkTaskGroup tg;" down to just before the for loop,
> then
> > remove the call to tg.wait()?
> > 
> > runnableDiffs goes out of scope before tg, so it's deleted before we call
> > wait().
> 
> I agree, that should fix the problem and I'll try it to see. 
> IMHO though it's desirable to call wait() explicitly rather than relying on
the
> destructor. 
> Maybe that's not a problem, but it would be nice if we could enforce that
> somehow, i.e. the destructor issue a warning before the call to wait.

Sgtm.  lgtm

[stephana, 2014-09-05 20:05:32]

The CQ bit was checked by stephana@google.com

[stephana, 2014-09-05 20:05:34]

The CQ bit was checked by stephana@google.com

[stephana, 2014-09-05 20:05:35]

The CQ bit was unchecked by stephana@google.com

[stephana, 2014-09-05 20:05:47]

The CQ bit was checked by stephana@google.com

[commit-bot: I haz the power, 2014-09-05 20:06:10]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/stephana@google.com/546753003/20001

[commit-bot: I haz the power, 2014-09-05 20:21:46]

Committed patchset #2 (id:20001) as 7260d7292bde966f038b8e166f3fe41ddd8f2099