Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1107573003:
Oilpan: Add a missing null check for Platform::current() (Closed)
|
Created:
5 years, 8 months ago by haraken Modified:
5 years, 6 months ago CC:
Mads Ager (chromium), blink-reviews, kouhei+heap_chromium.org, oilpan-reviews Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionOilpan: Add a missing null check for Platform::current()
Platform::current() can be null in webkit_unit_tests.
BUG=474470
Patch Set 1 #Patch Set 2 : #
Total comments: 1
Messages
Total messages: 24 (4 generated)
haraken@chromium.org changed reviewers: + oilpan-reviews@chromium.org, sigbjornf@opera.com
PTAL I'll post another fix to avoid Oilpan's GC from getting triggered without allocating any object in Oilpan's heap.
https://codereview.chromium.org/1107573003/diff/20001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1107573003/diff/20001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:666: if (Platform::current()) { Platform::current() will exist, but it may not have a thread (cf. initializeWithoutV8())
The tests that fail run with Platform::current() bound to an AnimationCompositorAnimationsTestBase::PlatformProxy instance. I am not familiar with the constraints that code which the compositor thread runs needs to obey, so let me ask some clarifying quesions: - Should it be possible for animations code on the compositor thread to allocate on the Oilpan heap? - Is that PlatformProxy object representative of the Platform object that animations do run in the context of? If no to the 2nd question, then perhaps this test setup needs to change and we can instead assert for Platform::current()->currentThread() being non-null before accessing?
haraken@chromium.org changed reviewers: + dstockwell@chromium.org, timloh@chromium.org
+dstockwell, +timloh: Do you have any idea on the comment #5?
dstockwell@chromium.org changed reviewers: + loyso@chromium.org, mithro@mithis.com - timloh@chromium.org
+mithro, +loyso, I'd assume the only point of PlatformProxy there is to wire up the compositorSupport() function, but I don't really know.
On 2015/04/24 at 05:47:44, dstockwell wrote: > +mithro, +loyso, > > I'd assume the only point of PlatformProxy there is to wire up the compositorSupport() function, but I don't really know. This likely has nothing to do with the compositor thread. It's just the blink code that hands off animation information to the chromium compositor.
On 2015/04/24 05:49:04, dstockwell wrote: > On 2015/04/24 at 05:47:44, dstockwell wrote: > > +mithro, +loyso, > > > > I'd assume the only point of PlatformProxy there is to wire up the > compositorSupport() function, but I don't really know. > > This likely has nothing to do with the compositor thread. It's just the blink > code that hands off animation information to the chromium compositor. ok, thanks - then i suggest we extend PlatformProxy to wire up a currentThread() also for the benefit of Oilpan.
mithro@mithis.com changed reviewers: + skyostil@chromium.org
+skyostil This method is part of the Blink Scheduler. It is for scheduling idle work such as garbage collection during periods we are not rendering. You'll want someone from the London scheduler team to comment on what having this missing might mean. I'm adding skyostil to take a look here. Tim
Just random note: Sometimes we need non-null Platform::current() to be able to run a code with tracing macroses. See this CL as an example: https://codereview.chromium.org/881183003/ It adds test for AnimationPlayer and fixes PlatformProxy. As for Oilpan Heap for cc/animation - that's unanswered question. We have it in our research plan as a separate (very late) stage for the animation engine refactoring.
On 2015/04/24 05:54:44, sof wrote: > On 2015/04/24 05:49:04, dstockwell wrote: > > On 2015/04/24 at 05:47:44, dstockwell wrote: > > > +mithro, +loyso, > > > > > > I'd assume the only point of PlatformProxy there is to wire up the > > compositorSupport() function, but I don't really know. > > > > This likely has nothing to do with the compositor thread. It's just the blink > > code that hands off animation information to the chromium compositor. > > ok, thanks - then i suggest we extend PlatformProxy to wire up a currentThread() > also for the benefit of Oilpan. https://codereview.chromium.org/1052103004/ does this.
On 2015/04/24 09:53:31, sof wrote: > On 2015/04/24 05:54:44, sof wrote: > > On 2015/04/24 05:49:04, dstockwell wrote: > > > On 2015/04/24 at 05:47:44, dstockwell wrote: > > > > +mithro, +loyso, > > > > > > > > I'd assume the only point of PlatformProxy there is to wire up the > > > compositorSupport() function, but I don't really know. > > > > > > This likely has nothing to do with the compositor thread. It's just the > blink > > > code that hands off animation information to the chromium compositor. > > > > ok, thanks - then i suggest we extend PlatformProxy to wire up a > currentThread() > > also for the benefit of Oilpan. > > https://codereview.chromium.org/1052103004/ does this. Thanks for the CL. That looks reasonable to me but should be confirmed by experts. BTW, what do you think about this CL? We normally have a null check for Platform::current() because Platform::current() can be null in some unit tests. (I'm wondering why we don't hit crashes without having the null check in scheduleIdleGC().)
On 2015/04/24 10:11:04, haraken wrote: > On 2015/04/24 09:53:31, sof wrote: > > On 2015/04/24 05:54:44, sof wrote: > > > On 2015/04/24 05:49:04, dstockwell wrote: > > > > On 2015/04/24 at 05:47:44, dstockwell wrote: > > > > > +mithro, +loyso, > > > > > > > > > > I'd assume the only point of PlatformProxy there is to wire up the > > > > compositorSupport() function, but I don't really know. > > > > > > > > This likely has nothing to do with the compositor thread. It's just the > > blink > > > > code that hands off animation information to the chromium compositor. > > > > > > ok, thanks - then i suggest we extend PlatformProxy to wire up a > > currentThread() > > > also for the benefit of Oilpan. > > > > https://codereview.chromium.org/1052103004/ does this. > > Thanks for the CL. That looks reasonable to me but should be confirmed by > experts. > > BTW, what do you think about this CL? We normally have a null check for > Platform::current() because Platform::current() can be null in some unit tests. > (I'm wondering why we don't hit crashes without having the null check in > scheduleIdleGC().) It depends, do we want to support running Oilpan not attached to an event loop that can service GCs? If so, then null checking currentThread() here when scheduling GCs makes sense. (The code has been using Scheduler::shared() up until recently.)
On 2015/04/24 10:20:04, sof wrote: > On 2015/04/24 10:11:04, haraken wrote: > > On 2015/04/24 09:53:31, sof wrote: > > > On 2015/04/24 05:54:44, sof wrote: > > > > On 2015/04/24 05:49:04, dstockwell wrote: > > > > > On 2015/04/24 at 05:47:44, dstockwell wrote: > > > > > > +mithro, +loyso, > > > > > > > > > > > > I'd assume the only point of PlatformProxy there is to wire up the > > > > > compositorSupport() function, but I don't really know. > > > > > > > > > > This likely has nothing to do with the compositor thread. It's just the > > > blink > > > > > code that hands off animation information to the chromium compositor. > > > > > > > > ok, thanks - then i suggest we extend PlatformProxy to wire up a > > > currentThread() > > > > also for the benefit of Oilpan. > > > > > > https://codereview.chromium.org/1052103004/ does this. > > > > Thanks for the CL. That looks reasonable to me but should be confirmed by > > experts. > > > > BTW, what do you think about this CL? We normally have a null check for > > Platform::current() because Platform::current() can be null in some unit > tests. > > (I'm wondering why we don't hit crashes without having the null check in > > scheduleIdleGC().) > > It depends, do we want to support running Oilpan not attached to an event loop > that can service GCs? If so, then null checking currentThread() here when > scheduling GCs makes sense. > > (The code has been using Scheduler::shared() up until recently.) Hmm, I'm a bit confused. If Platform::current() does not exist (in some unittests), Platform::current()->currentThread() crashes, doesn't it? I'm wondering why that doesn't happen here.
On 2015/04/24 10:48:36, haraken wrote: > On 2015/04/24 10:20:04, sof wrote: > > On 2015/04/24 10:11:04, haraken wrote: > > > On 2015/04/24 09:53:31, sof wrote: > > > > On 2015/04/24 05:54:44, sof wrote: > > > > > On 2015/04/24 05:49:04, dstockwell wrote: > > > > > > On 2015/04/24 at 05:47:44, dstockwell wrote: > > > > > > > +mithro, +loyso, > > > > > > > > > > > > > > I'd assume the only point of PlatformProxy there is to wire up the > > > > > > compositorSupport() function, but I don't really know. > > > > > > > > > > > > This likely has nothing to do with the compositor thread. It's just > the > > > > blink > > > > > > code that hands off animation information to the chromium compositor. > > > > > > > > > > ok, thanks - then i suggest we extend PlatformProxy to wire up a > > > > currentThread() > > > > > also for the benefit of Oilpan. > > > > > > > > https://codereview.chromium.org/1052103004/ does this. > > > > > > Thanks for the CL. That looks reasonable to me but should be confirmed by > > > experts. > > > > > > BTW, what do you think about this CL? We normally have a null check for > > > Platform::current() because Platform::current() can be null in some unit > > tests. > > > (I'm wondering why we don't hit crashes without having the null check in > > > scheduleIdleGC().) > > > > It depends, do we want to support running Oilpan not attached to an event loop > > that can service GCs? If so, then null checking currentThread() here when > > scheduling GCs makes sense. > > > > (The code has been using Scheduler::shared() up until recently.) > > Hmm, I'm a bit confused. If Platform::current() does not exist (in some > unittests), Platform::current()->currentThread() crashes, doesn't it? I'm > wondering why that doesn't happen here. If Blink is initialized, Platform::current() will be non-null. I don't think we should support running Oilpan without Blink initialized.
On 2015/04/24 10:56:26, sof wrote: > On 2015/04/24 10:48:36, haraken wrote: > > On 2015/04/24 10:20:04, sof wrote: > > > On 2015/04/24 10:11:04, haraken wrote: > > > > On 2015/04/24 09:53:31, sof wrote: > > > > > On 2015/04/24 05:54:44, sof wrote: > > > > > > On 2015/04/24 05:49:04, dstockwell wrote: > > > > > > > On 2015/04/24 at 05:47:44, dstockwell wrote: > > > > > > > > +mithro, +loyso, > > > > > > > > > > > > > > > > I'd assume the only point of PlatformProxy there is to wire up the > > > > > > > compositorSupport() function, but I don't really know. > > > > > > > > > > > > > > This likely has nothing to do with the compositor thread. It's just > > the > > > > > blink > > > > > > > code that hands off animation information to the chromium > compositor. > > > > > > > > > > > > ok, thanks - then i suggest we extend PlatformProxy to wire up a > > > > > currentThread() > > > > > > also for the benefit of Oilpan. > > > > > > > > > > https://codereview.chromium.org/1052103004/ does this. > > > > > > > > Thanks for the CL. That looks reasonable to me but should be confirmed by > > > > experts. > > > > > > > > BTW, what do you think about this CL? We normally have a null check for > > > > Platform::current() because Platform::current() can be null in some unit > > > tests. > > > > (I'm wondering why we don't hit crashes without having the null check in > > > > scheduleIdleGC().) > > > > > > It depends, do we want to support running Oilpan not attached to an event > loop > > > that can service GCs? If so, then null checking currentThread() here when > > > scheduling GCs makes sense. > > > > > > (The code has been using Scheduler::shared() up until recently.) > > > > Hmm, I'm a bit confused. If Platform::current() does not exist (in some > > unittests), Platform::current()->currentThread() crashes, doesn't it? I'm > > wondering why that doesn't happen here. > > If Blink is initialized, Platform::current() will be non-null. I don't think we > should support running Oilpan without Blink initialized. There had been a bug that Platform::current() doesn't exist in unit tests, but it seems that the issue is gone. So I think we can remove if(Platform::curren()) check from a couple of places in platform/heap/.
On 2015/04/24 11:03:27, haraken wrote: > On 2015/04/24 10:56:26, sof wrote: > > On 2015/04/24 10:48:36, haraken wrote: > > > On 2015/04/24 10:20:04, sof wrote: > > > > On 2015/04/24 10:11:04, haraken wrote: > > > > > On 2015/04/24 09:53:31, sof wrote: > > > > > > On 2015/04/24 05:54:44, sof wrote: > > > > > > > On 2015/04/24 05:49:04, dstockwell wrote: > > > > > > > > On 2015/04/24 at 05:47:44, dstockwell wrote: > > > > > > > > > +mithro, +loyso, > > > > > > > > > > > > > > > > > > I'd assume the only point of PlatformProxy there is to wire up > the > > > > > > > > compositorSupport() function, but I don't really know. > > > > > > > > > > > > > > > > This likely has nothing to do with the compositor thread. It's > just > > > the > > > > > > blink > > > > > > > > code that hands off animation information to the chromium > > compositor. > > > > > > > > > > > > > > ok, thanks - then i suggest we extend PlatformProxy to wire up a > > > > > > currentThread() > > > > > > > also for the benefit of Oilpan. > > > > > > > > > > > > https://codereview.chromium.org/1052103004/ does this. > > > > > > > > > > Thanks for the CL. That looks reasonable to me but should be confirmed > by > > > > > experts. > > > > > > > > > > BTW, what do you think about this CL? We normally have a null check for > > > > > Platform::current() because Platform::current() can be null in some unit > > > > tests. > > > > > (I'm wondering why we don't hit crashes without having the null check in > > > > > scheduleIdleGC().) > > > > > > > > It depends, do we want to support running Oilpan not attached to an event > > loop > > > > that can service GCs? If so, then null checking currentThread() here when > > > > scheduling GCs makes sense. > > > > > > > > (The code has been using Scheduler::shared() up until recently.) > > > > > > Hmm, I'm a bit confused. If Platform::current() does not exist (in some > > > unittests), Platform::current()->currentThread() crashes, doesn't it? I'm > > > wondering why that doesn't happen here. > > > > If Blink is initialized, Platform::current() will be non-null. I don't think > we > > should support running Oilpan without Blink initialized. > > There had been a bug that Platform::current() doesn't exist in unit tests, but > it seems that the issue is gone. So I think we can remove if(Platform::curren()) > check from a couple of places in platform/heap/. yes, sounds good -- as loyso@ said, some non-Blink (?) unit tests touch Blink code with tracing macro annotations, which requires a Platform::current() null check for the event tracing functions. But that's about it as far as supported non-Blink initialized usage, afaik.
On 2015/04/24 11:07:47, sof wrote: > On 2015/04/24 11:03:27, haraken wrote: > > On 2015/04/24 10:56:26, sof wrote: > > > On 2015/04/24 10:48:36, haraken wrote: > > > > On 2015/04/24 10:20:04, sof wrote: > > > > > On 2015/04/24 10:11:04, haraken wrote: > > > > > > On 2015/04/24 09:53:31, sof wrote: > > > > > > > On 2015/04/24 05:54:44, sof wrote: > > > > > > > > On 2015/04/24 05:49:04, dstockwell wrote: > > > > > > > > > On 2015/04/24 at 05:47:44, dstockwell wrote: > > > > > > > > > > +mithro, +loyso, > > > > > > > > > > > > > > > > > > > > I'd assume the only point of PlatformProxy there is to wire up > > the > > > > > > > > > compositorSupport() function, but I don't really know. > > > > > > > > > > > > > > > > > > This likely has nothing to do with the compositor thread. It's > > just > > > > the > > > > > > > blink > > > > > > > > > code that hands off animation information to the chromium > > > compositor. > > > > > > > > > > > > > > > > ok, thanks - then i suggest we extend PlatformProxy to wire up a > > > > > > > currentThread() > > > > > > > > also for the benefit of Oilpan. > > > > > > > > > > > > > > https://codereview.chromium.org/1052103004/ does this. > > > > > > > > > > > > Thanks for the CL. That looks reasonable to me but should be confirmed > > by > > > > > > experts. > > > > > > > > > > > > BTW, what do you think about this CL? We normally have a null check > for > > > > > > Platform::current() because Platform::current() can be null in some > unit > > > > > tests. > > > > > > (I'm wondering why we don't hit crashes without having the null check > in > > > > > > scheduleIdleGC().) > > > > > > > > > > It depends, do we want to support running Oilpan not attached to an > event > > > loop > > > > > that can service GCs? If so, then null checking currentThread() here > when > > > > > scheduling GCs makes sense. > > > > > > > > > > (The code has been using Scheduler::shared() up until recently.) > > > > > > > > Hmm, I'm a bit confused. If Platform::current() does not exist (in some > > > > unittests), Platform::current()->currentThread() crashes, doesn't it? I'm > > > > wondering why that doesn't happen here. > > > > > > If Blink is initialized, Platform::current() will be non-null. I don't think > > we > > > should support running Oilpan without Blink initialized. > > > > There had been a bug that Platform::current() doesn't exist in unit tests, but > > it seems that the issue is gone. So I think we can remove > if(Platform::curren()) > > check from a couple of places in platform/heap/. > > yes, sounds good -- as loyso@ said, some non-Blink (?) unit tests touch Blink > code with tracing macro annotations, which requires a Platform::current() null > check for the event tracing functions. But that's about it as far as supported > non-Blink initialized usage, afaik. My €.02 is that the fewer test mode checks we have in production code the better. This particular CL won't break idle scheduling, but if we can avoid having the Platform::current() check there in the first place that would be better.
On 2015/04/24 12:34:07, Sami wrote: > On 2015/04/24 11:07:47, sof wrote: > > On 2015/04/24 11:03:27, haraken wrote: > > > On 2015/04/24 10:56:26, sof wrote: > > > > On 2015/04/24 10:48:36, haraken wrote: > > > > > On 2015/04/24 10:20:04, sof wrote: > > > > > > On 2015/04/24 10:11:04, haraken wrote: > > > > > > > On 2015/04/24 09:53:31, sof wrote: > > > > > > > > On 2015/04/24 05:54:44, sof wrote: > > > > > > > > > On 2015/04/24 05:49:04, dstockwell wrote: > > > > > > > > > > On 2015/04/24 at 05:47:44, dstockwell wrote: > > > > > > > > > > > +mithro, +loyso, > > > > > > > > > > > > > > > > > > > > > > I'd assume the only point of PlatformProxy there is to wire > up > > > the > > > > > > > > > > compositorSupport() function, but I don't really know. > > > > > > > > > > > > > > > > > > > > This likely has nothing to do with the compositor thread. It's > > > just > > > > > the > > > > > > > > blink > > > > > > > > > > code that hands off animation information to the chromium > > > > compositor. > > > > > > > > > > > > > > > > > > ok, thanks - then i suggest we extend PlatformProxy to wire up a > > > > > > > > currentThread() > > > > > > > > > also for the benefit of Oilpan. > > > > > > > > > > > > > > > > https://codereview.chromium.org/1052103004/ does this. > > > > > > > > > > > > > > Thanks for the CL. That looks reasonable to me but should be > confirmed > > > by > > > > > > > experts. > > > > > > > > > > > > > > BTW, what do you think about this CL? We normally have a null check > > for > > > > > > > Platform::current() because Platform::current() can be null in some > > unit > > > > > > tests. > > > > > > > (I'm wondering why we don't hit crashes without having the null > check > > in > > > > > > > scheduleIdleGC().) > > > > > > > > > > > > It depends, do we want to support running Oilpan not attached to an > > event > > > > loop > > > > > > that can service GCs? If so, then null checking currentThread() here > > when > > > > > > scheduling GCs makes sense. > > > > > > > > > > > > (The code has been using Scheduler::shared() up until recently.) > > > > > > > > > > Hmm, I'm a bit confused. If Platform::current() does not exist (in some > > > > > unittests), Platform::current()->currentThread() crashes, doesn't it? > I'm > > > > > wondering why that doesn't happen here. > > > > > > > > If Blink is initialized, Platform::current() will be non-null. I don't > think > > > we > > > > should support running Oilpan without Blink initialized. > > > > > > There had been a bug that Platform::current() doesn't exist in unit tests, > but > > > it seems that the issue is gone. So I think we can remove > > if(Platform::curren()) > > > check from a couple of places in platform/heap/. > > > > yes, sounds good -- as loyso@ said, some non-Blink (?) unit tests touch Blink > > code with tracing macro annotations, which requires a Platform::current() null > > check for the event tracing functions. But that's about it as far as supported > > non-Blink initialized usage, afaik. > > My €.02 is that the fewer test mode checks we have in production code the > better. This particular CL won't break idle scheduling, but if we can avoid > having the Platform::current() check there in the first place that would be > better. Previously we had the Platform::current() check in a bunch of places. As far as I grep, those are gone, so it seems that we no longer need the check.
On 2015/04/24 12:38:16, haraken wrote: > ... > > Previously we had the Platform::current() check in a bunch of places. As far as > I grep, those are gone, so it seems that we no longer need the check. so this CL can be closed? :) |
