Use trace wrapper in PopStateEvent

Don't store ScriptValue in PopStateEvent

Use TraceWrapperV8Reference instead. This CL doesn't remove
custom getter of PopStateEvent.state as more work will be
needed to remove custom bindings.

BUG=501866
Review-Url: https://codereview.chromium.org/2850133002
Cr-Commit-Position: refs/heads/master@{#468565}
Committed: https://chromium.googlesource.com/chromium/src/+/77301cfe98de369b7d72659d4a95014f1d4dc4b6

[bashi, 2017-05-01 08:45:29]

The CQ bit was checked by bashi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 08:45:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 10:49:02]

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

[commit-bot: I haz the power, 2017-05-01 10:49:03]

Dry run: This issue passed the CQ dry run.

[bashi, 2017-05-02 01:36:26]

Description was changed from

==========
WIP: Use trace wrapper in PopStateEvent

BUG=501866
==========

to

==========
Don't store ScriptValue in PopStateEvent

Use TraceWrapperV8Reference instead. This CL doesn't remove
custom getter of PopStateEvent.state as more work will be
needed to remove custom bindings.

BUG=501866
==========

[bashi, 2017-05-02 01:39:23]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org

[bashi, 2017-05-02 01:39:23]

PTAL

[haraken, 2017-05-02 04:44:32]

LGTM

https://codereview.chromium.org/2850133002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/PopStateEvent.cpp (right):

https://codereview.chromium.org/2850133002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PopStateEvent.cpp:65: return
ScriptValue(script_state, serialized->Deserialize(isolate));

What happens if:

1) an isolated world stores state_
2) the main world accesses the state_
3) the main world accesses the state_
4) the main world accesses the state_
...

Do we need to serialize & deserialize the state_ for 2), 3), 4) etc every time?
I'm wondering if there's a way to speed up the main world case.

[bashi, 2017-05-02 05:08:52]

Let me submit this CL for now. Probably we should come up with a pattern to
avoid deserialization every time getters are accessed.

https://codereview.chromium.org/2850133002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/PopStateEvent.cpp (right):

https://codereview.chromium.org/2850133002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PopStateEvent.cpp:65: return
ScriptValue(script_state, serialized->Deserialize(isolate));
On 2017/05/02 04:44:32, haraken wrote:
> 
> What happens if:
> 
> 1) an isolated world stores state_
> 2) the main world accesses the state_
> 3) the main world accesses the state_
> 4) the main world accesses the state_
> ...
> 
> Do we need to serialize & deserialize the state_ for 2), 3), 4) etc every
time?
> I'm wondering if there's a way to speed up the main world case.

Yeah, that's a valid concern when we remove custom getter for |state|.  In
V8PopStateEvent::stateAttributeGetterCustom() we currently cache the result of
deserialization so I guess that 3) and 4) wouldn't take much time.

Other Events like ErrorEvent have the same issue so we may want to develop a
common pattern to address the issue. I'm not sure we already have a way to
address the issue.

[bashi, 2017-05-02 05:09:49]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2017-05-02 05:10:09]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 05:14:39]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1493701789241200, "parent_rev":
"4ae3646b574b83fec0213bab924259cf9d4b130f", "commit_rev":
"77301cfe98de369b7d72659d4a95014f1d4dc4b6"}

[commit-bot: I haz the power, 2017-05-02 05:15:15]

Description was changed from

==========
Don't store ScriptValue in PopStateEvent

Use TraceWrapperV8Reference instead. This CL doesn't remove
custom getter of PopStateEvent.state as more work will be
needed to remove custom bindings.

BUG=501866
==========

to

==========
Don't store ScriptValue in PopStateEvent

Use TraceWrapperV8Reference instead. This CL doesn't remove
custom getter of PopStateEvent.state as more work will be
needed to remove custom bindings.

BUG=501866

Review-Url: https://codereview.chromium.org/2850133002
Cr-Commit-Position: refs/heads/master@{#468565}
Committed:
https://chromium.googlesource.com/chromium/src/+/77301cfe98de369b7d72659d4a95...
==========

[commit-bot: I haz the power, 2017-05-02 05:15:16]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/77301cfe98de369b7d72659d4a95...

[haraken, 2017-05-02 05:16:08]

On 2017/05/02 05:08:52, bashi wrote:
> Let me submit this CL for now. Probably we should come up with a pattern to
> avoid deserialization every time getters are accessed.
> 
>
https://codereview.chromium.org/2850133002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/events/PopStateEvent.cpp (right):
> 
>
https://codereview.chromium.org/2850133002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/events/PopStateEvent.cpp:65: return
> ScriptValue(script_state, serialized->Deserialize(isolate));
> On 2017/05/02 04:44:32, haraken wrote:
> > 
> > What happens if:
> > 
> > 1) an isolated world stores state_
> > 2) the main world accesses the state_
> > 3) the main world accesses the state_
> > 4) the main world accesses the state_
> > ...
> > 
> > Do we need to serialize & deserialize the state_ for 2), 3), 4) etc every
> time?
> > I'm wondering if there's a way to speed up the main world case.
> 
> Yeah, that's a valid concern when we remove custom getter for |state|.  In
> V8PopStateEvent::stateAttributeGetterCustom() we currently cache the result of
> deserialization so I guess that 3) and 4) wouldn't take much time.
> 
> Other Events like ErrorEvent have the same issue so we may want to develop a
> common pattern to address the issue. I'm not sure we already have a way to
> address the issue.

Sounds good.

Just to clarify: We don't really care about the performance of an isolated
world. However, the main world case must be super fast.

[bashi, 2017-05-02 05:22:38]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2856683003/ by bashi@chromium.org.

The reason for reverting is: Broke builds.

[findit-for-me, 2017-05-02 05:34:40]

Findit (https://goo.gl/kROfz5) confirmed this CL at revision 468565 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[Michael Lippautz, 2017-05-02 06:47:58]

Where's the entry point for tracing the PopStateEvent, i.e., from where is it
traced?

[bashi, 2017-05-02 07:33:51]

On 2017/05/02 06:47:58, Michael Lippautz wrote:
> Where's the entry point for tracing the PopStateEvent, i.e., from where is it
> traced?

To be honest I'm not sure where is the entry point. haraken-san?

(I reverted this CL because this wasn't rebased correctly. Trying to re-land
https://codereview.chromium.org/2850383002/)

[haraken, 2017-05-02 08:04:50]

On 2017/05/02 07:33:51, bashi (ooo til May 8) wrote:
> On 2017/05/02 06:47:58, Michael Lippautz wrote:
> > Where's the entry point for tracing the PopStateEvent, i.e., from where is
it
> > traced?
> 
> To be honest I'm not sure where is the entry point. haraken-san?
> 
> (I reverted this CL because this wasn't rebased correctly. Trying to re-land
> https://codereview.chromium.org/2850383002/)

PopStateEvent is a ScriptWrappble, so it's naturally traced by V8, isn't it?

BTW it would be nicer to have a test for this. We can use
window.internals.GCObservation to check that a given wrapper is not gone.

[bashi, 2017-05-02 08:07:43]

On 2017/05/02 08:04:50, haraken wrote:
> On 2017/05/02 07:33:51, bashi (ooo til May 8) wrote:
> > On 2017/05/02 06:47:58, Michael Lippautz wrote:
> > > Where's the entry point for tracing the PopStateEvent, i.e., from where is
> it
> > > traced?
> > 
> > To be honest I'm not sure where is the entry point. haraken-san?
> > 
> > (I reverted this CL because this wasn't rebased correctly. Trying to re-land
> > https://codereview.chromium.org/2850383002/)
> 
> PopStateEvent is a ScriptWrappble, so it's naturally traced by V8, isn't it?
> 
> BTW it would be nicer to have a test for this. We can use
> window.internals.GCObservation to check that a given wrapper is not gone.

Will try to add a test next week (ping me if I forget :)

[Yuki, 2017-05-10 10:38:36]

On 2017/05/02 08:04:50, haraken wrote:
> On 2017/05/02 07:33:51, bashi (ooo til May 8) wrote:
> > On 2017/05/02 06:47:58, Michael Lippautz wrote:
> > > Where's the entry point for tracing the PopStateEvent, i.e., from where is
> it
> > > traced?
> > 
> > To be honest I'm not sure where is the entry point. haraken-san?
> > 
> > (I reverted this CL because this wasn't rebased correctly. Trying to re-land
> > https://codereview.chromium.org/2850383002/)
> 
> PopStateEvent is a ScriptWrappble, so it's naturally traced by V8, isn't it?

Is it really true?  V8 shouldn't trace something that is unreachable from the
root set.  I'm curious what has a reference to an Event and make it alive.

[haraken, 2017-05-10 10:42:18]

On 2017/05/10 10:38:36, Yuki wrote:
> On 2017/05/02 08:04:50, haraken wrote:
> > On 2017/05/02 07:33:51, bashi (ooo til May 8) wrote:
> > > On 2017/05/02 06:47:58, Michael Lippautz wrote:
> > > > Where's the entry point for tracing the PopStateEvent, i.e., from where
is
> > it
> > > > traced?
> > > 
> > > To be honest I'm not sure where is the entry point. haraken-san?
> > > 
> > > (I reverted this CL because this wasn't rebased correctly. Trying to
re-land
> > > https://codereview.chromium.org/2850383002/)
> > 
> > PopStateEvent is a ScriptWrappble, so it's naturally traced by V8, isn't it?
> 
> Is it really true?  V8 shouldn't trace something that is unreachable from the
> root set.  I'm curious what has a reference to an Event and make it alive.

The Event wrapper has a reference to the C++ Event object.

If the Event wrapper is reachable, the C++ Event object is traced. Otherwise,
the C++ Event object is not traced.

Am I misunderstanding?

[Yuki, 2017-05-10 10:50:16]

On 2017/05/10 10:42:18, haraken wrote:
> On 2017/05/10 10:38:36, Yuki wrote:
> > On 2017/05/02 08:04:50, haraken wrote:
> > > On 2017/05/02 07:33:51, bashi (ooo til May 8) wrote:
> > > > On 2017/05/02 06:47:58, Michael Lippautz wrote:
> > > > > Where's the entry point for tracing the PopStateEvent, i.e., from
where
> is
> > > it
> > > > > traced?
> > > > 
> > > > To be honest I'm not sure where is the entry point. haraken-san?
> > > > 
> > > > (I reverted this CL because this wasn't rebased correctly. Trying to
> re-land
> > > > https://codereview.chromium.org/2850383002/)
> > > 
> > > PopStateEvent is a ScriptWrappble, so it's naturally traced by V8, isn't
it?
> > 
> > Is it really true?  V8 shouldn't trace something that is unreachable from
the
> > root set.  I'm curious what has a reference to an Event and make it alive.
> 
> The Event wrapper has a reference to the C++ Event object.
> 
> If the Event wrapper is reachable, the C++ Event object is traced. Otherwise,
> the C++ Event object is not traced.
> 
> Am I misunderstanding?

I'm wondering what makes the Event wrapper alive.

[haraken, 2017-05-10 11:18:07]

On 2017/05/10 10:50:16, Yuki wrote:
> On 2017/05/10 10:42:18, haraken wrote:
> > On 2017/05/10 10:38:36, Yuki wrote:
> > > On 2017/05/02 08:04:50, haraken wrote:
> > > > On 2017/05/02 07:33:51, bashi (ooo til May 8) wrote:
> > > > > On 2017/05/02 06:47:58, Michael Lippautz wrote:
> > > > > > Where's the entry point for tracing the PopStateEvent, i.e., from
> where
> > is
> > > > it
> > > > > > traced?
> > > > > 
> > > > > To be honest I'm not sure where is the entry point. haraken-san?
> > > > > 
> > > > > (I reverted this CL because this wasn't rebased correctly. Trying to
> > re-land
> > > > > https://codereview.chromium.org/2850383002/)
> > > > 
> > > > PopStateEvent is a ScriptWrappble, so it's naturally traced by V8, isn't
> it?
> > > 
> > > Is it really true?  V8 shouldn't trace something that is unreachable from
> the
> > > root set.  I'm curious what has a reference to an Event and make it alive.
> > 
> > The Event wrapper has a reference to the C++ Event object.
> > 
> > If the Event wrapper is reachable, the C++ Event object is traced.
Otherwise,
> > the C++ Event object is not traced.
> > 
> > Am I misunderstanding?
> 
> I'm wondering what makes the Event wrapper alive.

Ah, now I understand your question! You are right.

I thought about this a bit more and begin to think that this might not be a
trivial problem. Let me start a thread on platform-architecture-dev@.