Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
338533002:
Fix null pointer crash with adjustAndMark (Closed)
|
Created:
6 years, 6 months ago by Erik Corry Modified:
6 years, 6 months ago CC:
blink-reviews, kouhei+heap_chromium.org, oilpan-reviews Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionFix null pointer crash with adjustAndMark
R=ager@chromium.org, sigbjornf@opera.com
BUG=384246
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176166
Patch Set 1 #
Total comments: 5
Messages
Total messages: 19 (0 generated)
lgtm
LGTM
https://codereview.chromium.org/338533002/diff/1/Source/platform/heap/Visitor.h File Source/platform/heap/Visitor.h (right): https://codereview.chromium.org/338533002/diff/1/Source/platform/heap/Visitor... Source/platform/heap/Visitor.h:624: if (self) Would it be possible move this "up" a bit? i.e., we're paying for a null check on pointers that are otherwise guaranteed to be non-null now, right?
https://codereview.chromium.org/338533002/diff/1/Source/platform/heap/Visitor.h File Source/platform/heap/Visitor.h (right): https://codereview.chromium.org/338533002/diff/1/Source/platform/heap/Visitor... Source/platform/heap/Visitor.h:223: if (!t) I was thinking that this null check is enough. Isn't this mark() method called before we hit self->adjustAndMark(visitor)?
On 2014/06/13 09:14:16, haraken wrote: > https://codereview.chromium.org/338533002/diff/1/Source/platform/heap/Visitor.h > File Source/platform/heap/Visitor.h (right): > > https://codereview.chromium.org/338533002/diff/1/Source/platform/heap/Visitor... > Source/platform/heap/Visitor.h:223: if (!t) > > I was thinking that this null check is enough. Isn't this mark() method called > before we hit self->adjustAndMark(visitor)? See the stacktrace on the bug (384246)
On 2014/06/13 09:14:16, haraken wrote: > https://codereview.chromium.org/338533002/diff/1/Source/platform/heap/Visitor.h > File Source/platform/heap/Visitor.h (right): > > https://codereview.chromium.org/338533002/diff/1/Source/platform/heap/Visitor... > Source/platform/heap/Visitor.h:223: if (!t) > > I was thinking that this null check is enough. Isn't this mark() method called > before we hit self->adjustAndMark(visitor)? Not for the collection trace traits. See the stack trace in the bug report. As Sigbjorn suggests maybe we can lift the null check into the collection trace traits instead of having the check here where it can be a duplicate check when we go through the mark method that also has the null check.
https://codereview.chromium.org/338533002/diff/1/Source/platform/heap/Visitor.h File Source/platform/heap/Visitor.h (right): https://codereview.chromium.org/338533002/diff/1/Source/platform/heap/Visitor... Source/platform/heap/Visitor.h:223: if (!t) On 2014/06/13 09:14:16, haraken wrote: > > I was thinking that this null check is enough. Isn't this mark() method called > before we hit self->adjustAndMark(visitor)? Not for members in vector backing. https://codereview.chromium.org/338533002/diff/1/Source/platform/heap/Visitor... Source/platform/heap/Visitor.h:624: if (self) On 2014/06/13 09:12:54, sof wrote: > Would it be possible move this "up" a bit? i.e., we're paying for a null check > on pointers that are otherwise guaranteed to be non-null now, right? This is just for things that need adjustAndMark, so I'm not too worried about the performance issues. Also I think once stuff has been inlined any extra checks are pretty simple to optimize out.
On 2014/06/13 09:40:27, Erik Corry wrote: > https://codereview.chromium.org/338533002/diff/1/Source/platform/heap/Visitor.h > File Source/platform/heap/Visitor.h (right): > > https://codereview.chromium.org/338533002/diff/1/Source/platform/heap/Visitor... > Source/platform/heap/Visitor.h:223: if (!t) > On 2014/06/13 09:14:16, haraken wrote: > > > > I was thinking that this null check is enough. Isn't this mark() method called > > before we hit self->adjustAndMark(visitor)? > > Not for members in vector backing. > > https://codereview.chromium.org/338533002/diff/1/Source/platform/heap/Visitor... > Source/platform/heap/Visitor.h:624: if (self) > On 2014/06/13 09:12:54, sof wrote: > > Would it be possible move this "up" a bit? i.e., we're paying for a null check > > on pointers that are otherwise guaranteed to be non-null now, right? > > This is just for things that need adjustAndMark, so I'm not too worried about > the performance issues. Also I think once stuff has been inlined any extra > checks are pretty simple to optimize out. LGTM.
lgtm https://codereview.chromium.org/338533002/diff/1/Source/platform/heap/Visitor.h File Source/platform/heap/Visitor.h (right): https://codereview.chromium.org/338533002/diff/1/Source/platform/heap/Visitor... Source/platform/heap/Visitor.h:624: if (self) On 2014/06/13 09:40:26, Erik Corry wrote: > On 2014/06/13 09:12:54, sof wrote: > > Would it be possible move this "up" a bit? i.e., we're paying for a null check > > on pointers that are otherwise guaranteed to be non-null now, right? > > This is just for things that need adjustAndMark, so I'm not too worried about > the performance issues. Also I think once stuff has been inlined any extra > checks are pretty simple to optimize out. alright, perhaps so.
The CQ bit was checked by erik.corry@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erik.corry@gmail.com/338533002/1
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
Looks like it stalled on bot flakiness; could someone send it along to green the oilpan bots some more? thanks :)
The CQ bit was checked by erik.corry@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erik.corry@gmail.com/338533002/1
Message was sent while issue was closed.
Change committed as 176166 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
