Oilpan: make custom element microtask dispatching lazy sweep savvy.

Oilpan: make custom element microtask dispatching lazy sweep savvy.

When microtask tasks are dispatched, the objects they work over might
be in the process of finalizing. Check before initiating the dispatch.

R=haraken
BUG=502855
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197646

[sof, 2015-06-23 09:11:54]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-06-23 09:11:54]

please take a look.

hmm.

[haraken, 2015-06-23 09:21:51]

On 2015/06/23 09:11:54, sof wrote:
> please take a look.
> 
> hmm.

hmm.

Maybe a better fix would be to add a pre-finalizer (or eager sweeping) to
CustomElementMicrotaskRunQueue and set a flag (or clear something) so that the
microtask that may run later doesn't dispatch anything (c.f., see how
ImageLoader's microtask is handled:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).

There are a couple of places that call Microtask::enqueueTask() and it looks
like we need take care of those call sites as well...

[sof, 2015-06-23 09:33:46]

On 2015/06/23 09:21:51, haraken wrote:
> On 2015/06/23 09:11:54, sof wrote:
> > please take a look.
> > 
> > hmm.
> 
> hmm.
> 
> Maybe a better fix would be to add a pre-finalizer (or eager sweeping) to
> CustomElementMicrotaskRunQueue and set a flag (or clear something) so that the
> microtask that may run later doesn't dispatch anything (c.f., see how
> ImageLoader's microtask is handled:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> 

(Eagerly) finalizing the queue won't de-reg it from the microtask run queue.

> There are a couple of places that call Microtask::enqueueTask() and it looks
> like we need take care of those call sites as well...

Feel free to jump in.

[haraken, 2015-06-23 09:48:10]

On 2015/06/23 09:33:46, sof wrote:
> On 2015/06/23 09:21:51, haraken wrote:
> > On 2015/06/23 09:11:54, sof wrote:
> > > please take a look.
> > > 
> > > hmm.
> > 
> > hmm.
> > 
> > Maybe a better fix would be to add a pre-finalizer (or eager sweeping) to
> > CustomElementMicrotaskRunQueue and set a flag (or clear something) so that
the
> > microtask that may run later doesn't dispatch anything (c.f., see how
> > ImageLoader's microtask is handled:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> > 
> 
> (Eagerly) finalizing the queue won't de-reg it from the microtask run queue.

Yes, so we can set a flag when the pre-finalizer runs so that the microtask::run
can determine whether it should really start running or not. In case of
ImageLoader, ImageLoader::Task::m_loader is used as a flag.

My point is that we want to avoid using Heap::willObjectBeLazilySwept because of
the unsafety we discussed before.

[sof, 2015-06-23 09:50:32]

On 2015/06/23 09:48:10, haraken wrote:
> On 2015/06/23 09:33:46, sof wrote:
> > On 2015/06/23 09:21:51, haraken wrote:
> > > On 2015/06/23 09:11:54, sof wrote:
> > > > please take a look.
> > > > 
> > > > hmm.
> > > 
> > > hmm.
> > > 
> > > Maybe a better fix would be to add a pre-finalizer (or eager sweeping) to
> > > CustomElementMicrotaskRunQueue and set a flag (or clear something) so that
> the
> > > microtask that may run later doesn't dispatch anything (c.f., see how
> > > ImageLoader's microtask is handled:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> > > 
> > 
> > (Eagerly) finalizing the queue won't de-reg it from the microtask run queue.
> 
> Yes, so we can set a flag when the pre-finalizer runs so that the
microtask::run
> can determine whether it should really start running or not. In case of
> ImageLoader, ImageLoader::Task::m_loader is used as a flag.
> 
> My point is that we want to avoid using Heap::willObjectBeLazilySwept because
of
> the unsafety we discussed before.

That is only an issue if you use it from within a destructor, which is not the
case here.

[sof, 2015-06-23 09:56:45]

We can reuse the WeakPtr<> for what it's purpose really is; eagerly clearing it
out now.

[haraken, 2015-06-23 10:25:11]

On 2015/06/23 09:56:45, sof wrote:
> We can reuse the WeakPtr<> for what it's purpose really is; eagerly clearing
it
> out now.

Looks nice! LGTM.

[sof, 2015-06-23 10:33:48]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-06-23 10:34:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1206503003/20001

[commit-bot: I haz the power, 2015-06-23 11:46:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-23 11:46:25]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/60265)

[sof, 2015-06-23 12:14:59]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-06-23 12:15:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1206503003/20001

[commit-bot: I haz the power, 2015-06-23 13:16:18]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197646

[sof, 2015-06-23 20:29:41]

On 2015/06/23 09:21:51, haraken wrote:
> On 2015/06/23 09:11:54, sof wrote:
> > please take a look.
> > 
> > hmm.
> 
> hmm.
> 
> Maybe a better fix would be to add a pre-finalizer (or eager sweeping) to
> CustomElementMicrotaskRunQueue and set a flag (or clear something) so that the
> microtask that may run later doesn't dispatch anything (c.f., see how
> ImageLoader's microtask is handled:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> 
> There are a couple of places that call Microtask::enqueueTask() and it looks
> like we need take care of those call sites as well...

The two remaining are MutationObserver and CustomElementMicrotaskDispatcher,
both checks out as not vulnerable:

 - MutationObserver dispatches over a set of observers kept alive by a global
set, hence it (the set) and the observers will all remain alive until
dispatched.
 - CustomElementMicrotaskDispatcher (CEMD) is similar, dispatching via a CEMD
singleton which keeps its callback queue objects alive.