Duplicate WindowProxy::disposeContext() into subclasses.

Duplicate WindowProxy::disposeContext() into subclasses.

Right now, the implementations are largely similar, but the
RemoteWindowProxy one will change substantially in followups.
Also clean up the includes for RemoteWindowProxy.

BUG=527190
Review-Url: https://codereview.chromium.org/2737553007
Cr-Commit-Position: refs/heads/master@{#455403}
Committed: https://chromium.googlesource.com/chromium/src/+/539c908fa2bb422de695ec180f5752ff909b91ae

[dcheng, 2017-03-08 06:08:08]

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

[commit-bot: I haz the power, 2017-03-08 06:08:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-03-08 06:09:32]

Another (hopefully) safe patch.

https://codereview.chromium.org/2737553007/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (left):

https://codereview.chromium.org/2737553007/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:31: #include
"bindings/core/v8/WindowProxy.h"
I guess I never fixed this copy and paste error...

https://codereview.chromium.org/2737553007/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:36: #include
"bindings/core/v8/ConditionalFeatures.h"
Trim these headers now since they're all unused.

[haraken, 2017-03-08 06:21:37]

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

[haraken, 2017-03-08 06:21:37]

LGTM

https://codereview.chromium.org/2737553007/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp (right):

https://codereview.chromium.org/2737553007/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:93:
V8DOMWrapper::clearNativeInfo(isolate(), context->Global());

Is your crash related to the fact that we're explicitly clearing an internal
field here? I think this is the only place we clear an internal field.

[commit-bot: I haz the power, 2017-03-08 06:36:29]

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

[commit-bot: I haz the power, 2017-03-08 06:36:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dcheng, 2017-03-08 06:46:41]

https://codereview.chromium.org/2737553007/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp (right):

https://codereview.chromium.org/2737553007/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:93:
V8DOMWrapper::clearNativeInfo(isolate(), context->Global());
On 2017/03/08 06:21:37, haraken wrote:
> 
> Is your crash related to the fact that we're explicitly clearing an internal
> field here? I think this is the only place we clear an internal field.

It's unclear. In theory, we shouldn't clear the internal fields without clearing
the wrapper class ID as well...

However, I think the DCHECK on line 87 has a hint to the problem. It seems like
it's possible to trigger situations where m_globalProxy != context->Global(). I
guess my WindowProxy CL must have made that more common.

Looking at the crash stack in
https://bugs.chromium.org/p/chromium/issues/detail?id=698739,
initializeIfNeeded() is crashing in the already-initialized branch. However, at
this point, we are in WebFrame::swap(): the frame we're swapping in should be
new, and should never have been initialized yet.

If it is initialized, it is a problem because:
- We call setGlobal() to transfer the global proxy during frame swap
- setGlobal() unconditionally overwrites m_globalProxy
- But context->Global() is already initialized and points to another global
proxy
- In teardown, we clear the wrapper class ID on m_globalProxy, but clear the
internal fields on context->Global().

Now, context->Global() has null internal fields but still has a wrapper class
ID.

+mlippautz, who mentioned that we only process non-null things from the marking
deque:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

But I'm not sure if that check would help here.

Regardless, I'm planning on investigating this by changing line 87/88 to CHECKs.
In WindowProxy::initializeIfNeeded(), I will capture a stack trace. This should
help us understand how we get into this situation.

[dcheng, 2017-03-08 06:48:00]

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

[commit-bot: I haz the power, 2017-03-08 06:48:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-03-08 06:50:48]

dcheng@chromium.org changed reviewers:
+ yukishiino@chromium.org

[dcheng, 2017-03-08 06:50:48]

+mlippautz for real this time (and +yukishiino who I left out by accident)

[Yuki, 2017-03-08 06:57:47]

LGTM.

[commit-bot: I haz the power, 2017-03-08 07:20:41]

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

[commit-bot: I haz the power, 2017-03-08 07:20:42]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dcheng, 2017-03-08 07:27:35]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2017-03-08 07:27:56]

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-03-08 08:39:21]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1488958055470580, "parent_rev":
"e4989441bc5665d3007e215a9d645ff73bc369df", "commit_rev":
"539c908fa2bb422de695ec180f5752ff909b91ae"}

[commit-bot: I haz the power, 2017-03-08 08:40:00]

Description was changed from

==========
Duplicate WindowProxy::disposeContext() into subclasses.

Right now, the implementations are largely similar, but the
RemoteWindowProxy one will change substantially in followups.
Also clean up the includes for RemoteWindowProxy.

BUG=527190
==========

to

==========
Duplicate WindowProxy::disposeContext() into subclasses.

Right now, the implementations are largely similar, but the
RemoteWindowProxy one will change substantially in followups.
Also clean up the includes for RemoteWindowProxy.

BUG=527190

Review-Url: https://codereview.chromium.org/2737553007
Cr-Commit-Position: refs/heads/master@{#455403}
Committed:
https://chromium.googlesource.com/chromium/src/+/539c908fa2bb422de695ec180f57...
==========

[commit-bot: I haz the power, 2017-03-08 08:40:09]

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