Remove DOMWindowProperty::willDetachGlobalObjectFromFrame

Remove DOMWindowProperty::willDetachGlobalObjectFromFrame

DOMWindowProperty::willDetachGlobalObjectFromFrame is dispatched
at the same timing as ContextLifecycleObserver::contextDestroyed
(via Document::detach). Hence this CL replaces DOMWindowProperty::willDetachGlobalObjectFromFrame
with ContextLifecycleObserver::contextDestroyed.

BUG=610176
Committed: https://crrev.com/f18ffebd279ad21196d988f763313e4a460db316
Cr-Commit-Position: refs/heads/master@{#404961}

[haraken, 2016-07-11 13:20:34]

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

[haraken, 2016-07-11 13:20:34]

PTAL

[dcheng, 2016-07-11 13:38:21]

LGTM, with one question

https://codereview.chromium.org/2138003002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/gamepad/NavigatorGamepad.cpp (right):

https://codereview.chromium.org/2138003002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/gamepad/NavigatorGamepad.cpp:119: if (document
&& document->frame() && document->frame()->host()) {
I wonder if it's possible to have document->frame() be true but
document->frame()->host() not be true...

I wonder if it would make sense to use document.isActive() here?

[haraken, 2016-07-11 23:18:21]

On 2016/07/11 13:38:21, dcheng wrote:
> LGTM, with one question
> 
>
https://codereview.chromium.org/2138003002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/gamepad/NavigatorGamepad.cpp (right):
> 
>
https://codereview.chromium.org/2138003002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/gamepad/NavigatorGamepad.cpp:119: if
(document
> && document->frame() && document->frame()->host()) {
> I wonder if it's possible to have document->frame() be true but
> document->frame()->host() not be true...

Yeah, document->frame()->host() doesn't make sense, so dropped.

> I wonder if it would make sense to use document.isActive() here?

Yeah. But NavigatorGamepad has a bunch of checks about document->frame(), so
I'll keep it as is.

[haraken, 2016-07-11 23:18:27]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-07-11 23:18:27]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2138003002/#ps20001
(title: "temp")

[commit-bot: I haz the power, 2016-07-11 23:19:34]

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, 2016-07-11 23:24:45]

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

[commit-bot: I haz the power, 2016-07-11 23:24:47]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[haraken, 2016-07-11 23:28:09]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-07-11 23:28:10]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2138003002/#ps40001
(title: "temp")

[commit-bot: I haz the power, 2016-07-11 23:29:13]

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, 2016-07-12 00:54:42]

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

[commit-bot: I haz the power, 2016-07-12 00:54:43]

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

[haraken, 2016-07-12 03:55:33]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-07-12 03:56:36]

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

[haraken, 2016-07-12 04:02:18]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-07-12 04:02:19]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2138003002/#ps60001
(title: "temp")

[commit-bot: I haz the power, 2016-07-12 04:02:58]

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, 2016-07-12 05:30:18]

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

[commit-bot: I haz the power, 2016-07-12 05:30:19]

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_...)

[haraken, 2016-07-12 06:10:08]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-07-12 06:10: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, 2016-07-12 08:57:59]

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

[commit-bot: I haz the power, 2016-07-12 08:58:00]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[haraken, 2016-07-12 09:06:47]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-07-12 09:07:11]

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, 2016-07-12 10:35:18]

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

[commit-bot: I haz the power, 2016-07-12 10:35:19]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[haraken, 2016-07-12 10:36:14]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-07-12 10:36:42]

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, 2016-07-12 12:59:14]

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

[commit-bot: I haz the power, 2016-07-12 12:59:15]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[haraken, 2016-07-13 00:04:11]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-07-13 00:07:30]

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, 2016-07-13 03:28:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-13 03:29:46]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 03:30:22]

Description was changed from

==========
Remove DOMWindowProperty::willDetachGlobalObjectFromFrame

DOMWindowProperty::willDetachGlobalObjectFromFrame is dispatched
at the same timing as ContextLifecycleObserver::contextDestroyed
(via Document::detach). Hence this CL replaces
DOMWindowProperty::willDetachGlobalObjectFromFrame
with ContextLifecycleObserver::contextDestroyed.

BUG=610176
==========

to

==========
Remove DOMWindowProperty::willDetachGlobalObjectFromFrame

DOMWindowProperty::willDetachGlobalObjectFromFrame is dispatched
at the same timing as ContextLifecycleObserver::contextDestroyed
(via Document::detach). Hence this CL replaces
DOMWindowProperty::willDetachGlobalObjectFromFrame
with ContextLifecycleObserver::contextDestroyed.

BUG=610176

Committed: https://crrev.com/f18ffebd279ad21196d988f763313e4a460db316
Cr-Commit-Position: refs/heads/master@{#404961}
==========

[commit-bot: I haz the power, 2016-07-13 03:30:23]

Patchset 4 (id:??) landed as
https://crrev.com/f18ffebd279ad21196d988f763313e4a460db316
Cr-Commit-Position: refs/heads/master@{#404961}

[falken, 2016-07-14 08:05:17]

On 2016/07/13 03:30:23, commit-bot: I haz the power wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/f18ffebd279ad21196d988f763313e4a460db316
> Cr-Commit-Position: refs/heads/master@{#404961}

After this patch serviceworker layout tests crash when dcheck is enabled:

$ third_party/WebKit/Tools/Scripts/run-webkit-tests --target=Default
--no-retry-failures http/tests/serviceworker
...
[68/146] http/tests/serviceworker/chromium/resolve-after-window-close.html
failed unexpectedly (renderer crashed)
[74/146] http/tests/serviceworker/getregistration.html failed unexpectedly
(renderer crashed)
[77/146] http/tests/serviceworker/chromium/service-worker-gc.html failed
unexpectedly (renderer crashed) 
[143/146] http/tests/serviceworker/update-served-from-cache.html failed
unexpectedly (renderer crashed)  

Reverting the patch fixes the problem locally. Shall I revert?

STDERR: ASSERTION FAILED: !m_provider
STDERR:
../../third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainer.cpp(182)
: virtual blink::ServiceWorkerContainer::~ServiceWorkerContainer()
STDERR: 1   0x7f33a93228bc
STDERR: 2   0x7f33a931f9e1 blink::BaseArena::sweepUnsweptPage()
STDERR: 3   0x7f33a931fc38 blink::BaseArena::completeSweep()
STDERR: 4   0x7f33a9326b8b blink::ThreadState::completeSweep()
STDERR: 5   0x7f33a9321dce blink::NormalPageArena::outOfLineAllocate(unsigned
long, unsigned long)

[haraken, 2016-07-14 08:06:42]

On 2016/07/14 08:05:17, falken wrote:
> On 2016/07/13 03:30:23, commit-bot: I haz the power wrote:
> > Patchset 4 (id:??) landed as
> > https://crrev.com/f18ffebd279ad21196d988f763313e4a460db316
> > Cr-Commit-Position: refs/heads/master@{#404961}
> 
> After this patch serviceworker layout tests crash when dcheck is enabled:
> 
> $ third_party/WebKit/Tools/Scripts/run-webkit-tests --target=Default
> --no-retry-failures http/tests/serviceworker
> ...
> [68/146] http/tests/serviceworker/chromium/resolve-after-window-close.html
> failed unexpectedly (renderer crashed)
> [74/146] http/tests/serviceworker/getregistration.html failed unexpectedly
> (renderer crashed)
> [77/146] http/tests/serviceworker/chromium/service-worker-gc.html failed
> unexpectedly (renderer crashed) 
> [143/146] http/tests/serviceworker/update-served-from-cache.html failed
> unexpectedly (renderer crashed)  
> 
> Reverting the patch fixes the problem locally. Shall I revert?
> 
> STDERR: ASSERTION FAILED: !m_provider
> STDERR:
>
../../third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainer.cpp(182)
> : virtual blink::ServiceWorkerContainer::~ServiceWorkerContainer()
> STDERR: 1   0x7f33a93228bc
> STDERR: 2   0x7f33a931f9e1 blink::BaseArena::sweepUnsweptPage()
> STDERR: 3   0x7f33a931fc38 blink::BaseArena::completeSweep()
> STDERR: 4   0x7f33a9326b8b blink::ThreadState::completeSweep()
> STDERR: 5   0x7f33a9321dce blink::NormalPageArena::outOfLineAllocate(unsigned
> long, unsigned long)

Taking a look.

[haraken, 2016-07-14 08:28:00]

On 2016/07/14 08:06:42, haraken wrote:
> On 2016/07/14 08:05:17, falken wrote:
> > On 2016/07/13 03:30:23, commit-bot: I haz the power wrote:
> > > Patchset 4 (id:??) landed as
> > > https://crrev.com/f18ffebd279ad21196d988f763313e4a460db316
> > > Cr-Commit-Position: refs/heads/master@{#404961}
> > 
> > After this patch serviceworker layout tests crash when dcheck is enabled:
> > 
> > $ third_party/WebKit/Tools/Scripts/run-webkit-tests --target=Default
> > --no-retry-failures http/tests/serviceworker
> > ...
> > [68/146] http/tests/serviceworker/chromium/resolve-after-window-close.html
> > failed unexpectedly (renderer crashed)
> > [74/146] http/tests/serviceworker/getregistration.html failed unexpectedly
> > (renderer crashed)
> > [77/146] http/tests/serviceworker/chromium/service-worker-gc.html failed
> > unexpectedly (renderer crashed) 
> > [143/146] http/tests/serviceworker/update-served-from-cache.html failed
> > unexpectedly (renderer crashed)  
> > 
> > Reverting the patch fixes the problem locally. Shall I revert?
> > 
> > STDERR: ASSERTION FAILED: !m_provider
> > STDERR:
> >
>
../../third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainer.cpp(182)
> > : virtual blink::ServiceWorkerContainer::~ServiceWorkerContainer()
> > STDERR: 1   0x7f33a93228bc
> > STDERR: 2   0x7f33a931f9e1 blink::BaseArena::sweepUnsweptPage()
> > STDERR: 3   0x7f33a931fc38 blink::BaseArena::completeSweep()
> > STDERR: 4   0x7f33a9326b8b blink::ThreadState::completeSweep()
> > STDERR: 5   0x7f33a9321dce
blink::NormalPageArena::outOfLineAllocate(unsigned
> > long, unsigned long)
> 
> Taking a look.

OK, let me revert this. Sorry about the trouble :/

[haraken, 2016-07-14 08:28:42]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2146343002/ by haraken@chromium.org.

The reason for reverting is: This CL caused flaky failures on
http/tests/serviceworker.
.

[haraken, 2016-07-14 09:04:06]

On 2016/07/14 08:28:42, haraken wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/2146343002/ by mailto:haraken@chromium.org.
> 
> The reason for reverting is: This CL caused flaky failures on
> http/tests/serviceworker.
> .

I uploaded a fix: https://codereview.chromium.org/2144213002/