Oilpan: Replace most of RefPtrs for Event objects with oilpan's transition types

Oilpan: Replace most of RefPtrs of Event objects with oilpan's transition types

- Basically, this CL does the following three things:

(1) Replace PassRefPtr<xxxEvent> with PassRefPtrWillBeRawPtr<xxxEvent>.

(2) Replace RefPtr<xxxEvent> in on-heap objects with RefPtrWillBeMember<xxxEvent>.

(3) Replace RefPtr<xxxEvent> in off-heap objects with RefPtrWillBePersistent<xxxEvent>.

- This CL is not replacing any raw pointers with Members/Persistents, so it won't produce new memory leaks.

- This CL uses ListHashSet<Persistent<Event>> for DOMWindowEventQueue::m_queuedEvents, because HeapListHashSet is not yet implemented.

- This CL uses Deque<Persistent<Event>> for WebSocket::m_events, because HeapDeque is not yet implemented.

- In follow-up CLs, I will move more Event related objects to the heap and reduce Persistent handles introduced in this CL.

BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170733

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170769

[haraken, 2014-03-28 06:57:28]

Not ready for review. I need to fix crashes and verify that persistent handles
in this CL won't cause leaks.

[haraken, 2014-03-28 12:14:32]

PTAL

[sof, 2014-03-28 12:27:17]

https://codereview.chromium.org/216523002/diff/20001/Source/bindings/v8/Worke...
File Source/bindings/v8/WorkerScriptController.h (right):

https://codereview.chromium.org/216523002/diff/20001/Source/bindings/v8/Worke...
Source/bindings/v8/WorkerScriptController.h:114: RefPtrWillBeRawPtr<ErrorEvent>
m_errorEventFromImportedScript;
Why is a raw pointer safe here? If it is, could you add an explanatory comment?

[sof, 2014-03-28 12:32:07]

https://codereview.chromium.org/216523002/diff/20001/public/web/WebDOMEvent.h
File public/web/WebDOMEvent.h (right):

https://codereview.chromium.org/216523002/diff/20001/public/web/WebDOMEvent.h...
public/web/WebDOMEvent.h:96: PassRefPtrWillBeRawPtr<WebCore::Event> getValue()
const;
Unused?

[haraken, 2014-03-28 14:02:13]

https://codereview.chromium.org/216523002/diff/20001/Source/bindings/v8/Worke...
File Source/bindings/v8/WorkerScriptController.h (right):

https://codereview.chromium.org/216523002/diff/20001/Source/bindings/v8/Worke...
Source/bindings/v8/WorkerScriptController.h:114: RefPtrWillBeRawPtr<ErrorEvent>
m_errorEventFromImportedScript;
On 2014/03/28 12:27:18, sof wrote:
> Why is a raw pointer safe here? If it is, could you add an explanatory
comment?

Good catch. This should be a Persistent. Fixed.

https://codereview.chromium.org/216523002/diff/20001/public/web/WebDOMEvent.h
File public/web/WebDOMEvent.h (right):

https://codereview.chromium.org/216523002/diff/20001/public/web/WebDOMEvent.h...
public/web/WebDOMEvent.h:96: PassRefPtrWillBeRawPtr<WebCore::Event> getValue()
const;
On 2014/03/28 12:32:09, sof wrote:
> Unused?

It's used in Source/web/ in this CL.

[sof, 2014-03-28 14:41:01]

I haven't been able to spot any other problems.

https://codereview.chromium.org/216523002/diff/20001/public/web/WebDOMEvent.h
File public/web/WebDOMEvent.h (right):

https://codereview.chromium.org/216523002/diff/20001/public/web/WebDOMEvent.h...
public/web/WebDOMEvent.h:96: PassRefPtrWillBeRawPtr<WebCore::Event> getValue()
const;
On 2014/03/28 14:02:14, haraken wrote:
> On 2014/03/28 12:32:09, sof wrote:
> > Unused?
> 
> It's used in Source/web/ in this CL.

Oh, so it is. Sorry about the noise.

[haraken, 2014-03-31 03:53:47]

Thanks sof for review!

I'd be happy if one more oilpan reviewer could take a look at the CL. PTAL.

[Mads Ager (chromium), 2014-03-31 06:38:49]

LGTM

We need to get those ListHashSets and Deques supported on-heap. :)

[haraken, 2014-03-31 08:03:53]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-03-31 08:03:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/216523002/40001

[commit-bot: I haz the power, 2014-03-31 08:17:33]

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

[commit-bot: I haz the power, 2014-03-31 08:17:33]

Try jobs failed on following builders:
  tryserver.blink on blink_presubmit

[haraken, 2014-03-31 08:55:20]

tkent-san: Could you take a look at web/ and public/? I'm no rush about this CL.

[haraken, 2014-04-01 08:34:30]

tkent-san: ping?

[tkent, 2014-04-02 06:22:00]

https://codereview.chromium.org/216523002/diff/40001/Source/core/events/DOMWi...
File Source/core/events/DOMWindowEventQueue.h (right):

https://codereview.chromium.org/216523002/diff/40001/Source/core/events/DOMWi...
Source/core/events/DOMWindowEventQueue.h:62:
ListHashSet<RefPtrWillBePersistent<Event>, 16> m_queuedEvents;
Does it work well even if rehash() happens?  If Persistent objects were copied
with memcpy or something, the persistent list would break.

https://codereview.chromium.org/216523002/diff/40001/public/web/WebDOMEvent.h
File public/web/WebDOMEvent.h (right):

https://codereview.chromium.org/216523002/diff/40001/public/web/WebDOMEvent.h...
public/web/WebDOMEvent.h:96: PassRefPtrWillBeRawPtr<WebCore::Event> getValue()
const;
Why don't you use |operator PassRefPtrWillBeRawPtr<>()| ?

Also, we don't prepend "get" to a simple accessor function.

[haraken, 2014-04-02 06:27:59]

https://codereview.chromium.org/216523002/diff/40001/Source/core/events/DOMWi...
File Source/core/events/DOMWindowEventQueue.h (right):

https://codereview.chromium.org/216523002/diff/40001/Source/core/events/DOMWi...
Source/core/events/DOMWindowEventQueue.h:62:
ListHashSet<RefPtrWillBePersistent<Event>, 16> m_queuedEvents;
On 2014/04/02 06:22:01, tkent wrote:
> Does it work well even if rehash() happens?  If Persistent objects were copied
> with memcpy or something, the persistent list would break.

Erik: Do you have any idea on this?

https://codereview.chromium.org/216523002/diff/40001/public/web/WebDOMEvent.h
File public/web/WebDOMEvent.h (right):

https://codereview.chromium.org/216523002/diff/40001/public/web/WebDOMEvent.h...
public/web/WebDOMEvent.h:96: PassRefPtrWillBeRawPtr<WebCore::Event> getValue()
const;
On 2014/04/02 06:22:01, tkent wrote:
> Why don't you use |operator PassRefPtrWillBeRawPtr<>()| ?
> 
> Also, we don't prepend "get" to a simple accessor function.

Will fix as you suggest.

In our experimental branch, we were replacing the implicit type converter with
getValue() for some complicated reason which I don't remember, but I guess the
issue is already gone when we deprecated Handle<> and Result<>.

[Mads Ager (chromium), 2014-04-02 06:31:44]

https://codereview.chromium.org/216523002/diff/40001/Source/core/events/DOMWi...
File Source/core/events/DOMWindowEventQueue.h (right):

https://codereview.chromium.org/216523002/diff/40001/Source/core/events/DOMWi...
Source/core/events/DOMWindowEventQueue.h:62:
ListHashSet<RefPtrWillBePersistent<Event>, 16> m_queuedEvents;
On 2014/04/02 06:22:01, tkent wrote:
> Does it work well even if rehash() happens?  If Persistent objects were copied
> with memcpy or something, the persistent list would break.

Yes, this works. For HashMaps, HashSets, ListHashSets objects are always
constructed and destructed. (Which means that a rehashing of a set of RefPtrs
will increment and decrement the ref count for every move.)

That said, we should get the ListHashSet supported on heap ASAP.

[haraken, 2014-04-02 09:02:32]

tkent-san: Thanks for review! Rebased with ToT. PTAL.

https://codereview.chromium.org/216523002/diff/40001/public/web/WebDOMEvent.h
File public/web/WebDOMEvent.h (right):

https://codereview.chromium.org/216523002/diff/40001/public/web/WebDOMEvent.h...
public/web/WebDOMEvent.h:96: PassRefPtrWillBeRawPtr<WebCore::Event> getValue()
const;
On 2014/04/02 06:28:00, haraken wrote:
> On 2014/04/02 06:22:01, tkent wrote:
> > Why don't you use |operator PassRefPtrWillBeRawPtr<>()| ?
> > 
> > Also, we don't prepend "get" to a simple accessor function.
> 
> Will fix as you suggest.
> 
> In our experimental branch, we were replacing the implicit type converter with
> getValue() for some complicated reason which I don't remember, but I guess the
> issue is already gone when we deprecated Handle<> and Result<>.

Done.

[tkent, 2014-04-02 09:10:48]

lgtm

[haraken, 2014-04-02 09:11:09]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-04-02 09:11:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/216523002/60001

[commit-bot: I haz the power, 2014-04-02 10:24:04]

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

[commit-bot: I haz the power, 2014-04-02 10:24:04]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[haraken, 2014-04-03 04:03:59]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-04-03 04:04:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/216523002/60001

[commit-bot: I haz the power, 2014-04-03 05:46:29]

Change committed as 170733

[haraken, 2014-04-03 10:39:30]

It turned out that the cause of the crashes caused by this CL was uninitialized
RawPtr's. In the patch set 5, I initialized all RawPtrs with '= nullptr's.

mads: I locally confirmed that all tests pass, but may I reland this CL? Or do
you want to split this?

https://codereview.chromium.org/216523002/diff/80001/Source/core/css/FontFace...
File Source/core/css/FontFaceSet.cpp (right):

https://codereview.chromium.org/216523002/diff/80001/Source/core/css/FontFace...
Source/core/css/FontFaceSet.cpp:411: RefPtrWillBeRawPtr<CSSFontFaceLoadEvent>
errorEvent = nullptr;

These '= nullptr's were necessary.

https://codereview.chromium.org/216523002/diff/80001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/216523002/diff/80001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.cpp:239: RefPtrWillBeRawPtr<ErrorEvent>
errorEvent = nullptr;

This '= nullptr' was necessary.

[Mads Ager (chromium), 2014-04-03 10:52:10]

If it passes all tests there is no reason to split it. Go ahead and land and we
will keep an eye on it. :)

[haraken, 2014-04-03 10:57:23]

The CQ bit was checked by haraken@chromium.org

[haraken, 2014-04-03 10:57:31]

On 2014/04/03 10:52:10, Mads Ager (chromium) wrote:
> If it passes all tests there is no reason to split it. Go ahead and land and
we
> will keep an eye on it. :)

Thanks, pushed into the commit queue.

[commit-bot: I haz the power, 2014-04-03 10:57:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/216523002/80001

[haraken, 2014-04-03 10:58:33]

On 2014/04/03 10:57:31, haraken wrote:
> On 2014/04/03 10:52:10, Mads Ager (chromium) wrote:
> > If it passes all tests there is no reason to split it. Go ahead and land and
> we
> > will keep an eye on it. :)
> 
> Thanks, pushed into the commit queue.

Note: The patch set 5 contains https://codereview.chromium.org/223163004/

[commit-bot: I haz the power, 2014-04-03 12:05:08]

Change committed as 170769