Clear LifecycleObserver::m_context when LifecycleObserver::contextDestroyed gets called

Clear LifecycleObserver::m_context when LifecycleObserver::contextDestroyed gets called

This CL clears LifecycleObserver::m_context when LifecycleObserver::contextDestroyed gets called.
This means that LifecycleObserver::context() starts returning false after the context gets destroyed.

The new behavior makes a lot of more sense, but the problem is that some existing code is assuming that
LifecycleObserver::context() keeps returning the context even after the context gets destroyed.
This CL fixed the problematic code to make all layout tests pass, but I'm not sure if I've updated all of the problematic code
(because the coverage of the layout tests is not sufficient).
However, the worst thing that can happen is that code that had been assuming that LifecycleObserver::context()
returns a context even after the context gets destroyed will stop working (it may cause a null-deref crash).
I think the risk is low.

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

[haraken, 2016-09-07 05:13:28]

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

[commit-bot: I haz the power, 2016-09-07 05:13:48]

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, 2016-09-07 05:24:19]

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

[commit-bot: I haz the power, 2016-09-07 05:24:20]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[haraken, 2016-09-07 06:38:21]

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

[commit-bot: I haz the power, 2016-09-07 06:38:37]

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, 2016-09-07 06:52:25]

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

[commit-bot: I haz the power, 2016-09-07 06:52:26]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[haraken, 2016-09-07 07:05:40]

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

[commit-bot: I haz the power, 2016-09-07 07:05:50]

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, 2016-09-07 07:18:03]

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

[commit-bot: I haz the power, 2016-09-07 07:18:04]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  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-09-07 07:31:59]

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

[commit-bot: I haz the power, 2016-09-07 07:32:14]

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, 2016-09-07 08:04:32]

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

[commit-bot: I haz the power, 2016-09-07 08:04:33]

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

[haraken, 2016-09-08 07:52:41]

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

[commit-bot: I haz the power, 2016-09-08 07:53:03]

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, 2016-09-08 08:56:38]

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

[commit-bot: I haz the power, 2016-09-08 08:56:39]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[haraken, 2016-09-08 09:43:30]

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

[commit-bot: I haz the power, 2016-09-08 09:43:44]

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, 2016-09-08 12:08:19]

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

[commit-bot: I haz the power, 2016-09-08 12:08:19]

Dry run: 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-09-09 04:29:11]

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

[commit-bot: I haz the power, 2016-09-09 04:29:22]

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, 2016-09-09 05:30:22]

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

[commit-bot: I haz the power, 2016-09-09 05:30:23]

Dry run: 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-09-15 01:44:24]

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

[commit-bot: I haz the power, 2016-09-15 01:45:01]

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, 2016-09-15 03:21:24]

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

[commit-bot: I haz the power, 2016-09-15 03:21:24]

Dry run: 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-09-15 12:38:06]

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

[commit-bot: I haz the power, 2016-09-15 12:38:22]

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, 2016-09-15 13:56:24]

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

[commit-bot: I haz the power, 2016-09-15 13:56:25]

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

[haraken, 2016-09-15 14:06:44]

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

[commit-bot: I haz the power, 2016-09-15 14:07:00]

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, 2016-09-15 15:27:57]

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

[commit-bot: I haz the power, 2016-09-15 15:27:58]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-09-15 15:54:52]

Description was changed from

==========
Clear LifecycleObserver::m_context when the context gets destroyed

BUG=
==========

to

==========
Clear LifecycleObserver::m_context when LifecycleObserver::contextDestroyed gets
called

This CL clears LifecycleObserver::m_context when
LifecycleObserver::contextDestroyed gets called.
This means that LifecycleObserver::context() starts returning false after the
context gets destroyed.

The new behavior makes a lot of more sense, but the problem is that some
existing code is assuming that
LifecycleObserver::context() keeps returning the context even after the context
gets destroyed.
This CL fixed the problematic code to make all layout tests pass, but I'm not
sure if I've updated all of the problematic code (because the coverage of the
layout tests is not sufficient).
However, the worst thing that can happen is that code that had been assuming
that LifecycleObserver::context()
returns a context even after the context gets destroyed will stop working (it
may cause a null-deref crash).
I think the risk is low.

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

[haraken, 2016-09-15 15:54:52]

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

[haraken, 2016-09-15 15:58:13]

PTAL

I'll send a PSA before landing this CL because it may cause null-deref crashes
(which I'll fix asap once I receive crash reports).

[dcheng, 2016-09-16 22:49:52]

https://codereview.chromium.org/2317483005/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/2317483005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:654:
throwForLoadFailureIfNeeded(exceptionState, "Document is already detached.");
Is this something required by the spec? I read
https://xhr.spec.whatwg.org/#the-send()-method, and I don't see any reference to
this.

[haraken, 2016-09-16 23:41:18]

On 2016/09/16 22:49:52, dcheng wrote:
>
https://codereview.chromium.org/2317483005/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):
> 
>
https://codereview.chromium.org/2317483005/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:654:
> throwForLoadFailureIfNeeded(exceptionState, "Document is already detached.");
> Is this something required by the spec? I read
> https://xhr.spec.whatwg.org/#the-send()-method, and I don't see any reference
to
> this.

Yeah, I couldn't find any description in the spec either.

However, the throwForLoadFailureIfNeeded is needed to pass
fast/xmlhttprequest/xmlhttprequest-detached-mixedcontent-no-crash.html. The
behavior also matches what Firefox does. In any case, it's needed to keep the
existing behavior.

[dcheng, 2016-09-17 00:17:41]

On 2016/09/16 23:41:18, haraken wrote:
> On 2016/09/16 22:49:52, dcheng wrote:
> >
>
https://codereview.chromium.org/2317483005/diff/200001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/2317483005/diff/200001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:654:
> > throwForLoadFailureIfNeeded(exceptionState, "Document is already
detached.");
> > Is this something required by the spec? I read
> > https://xhr.spec.whatwg.org/#the-send()-method, and I don't see any
reference
> to
> > this.
> 
> Yeah, I couldn't find any description in the spec either.
> 
> However, the throwForLoadFailureIfNeeded is needed to pass
> fast/xmlhttprequest/xmlhttprequest-detached-mixedcontent-no-crash.html. The
> behavior also matches what Firefox does. In any case, it's needed to keep the
> existing behavior.

What happens if we don't call this? The old code just returned false in this
case, I thought?

[haraken, 2016-09-17 00:47:00]

On 2016/09/17 00:17:41, dcheng wrote:
> On 2016/09/16 23:41:18, haraken wrote:
> > On 2016/09/16 22:49:52, dcheng wrote:
> > >
> >
>
https://codereview.chromium.org/2317483005/diff/200001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2317483005/diff/200001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:654:
> > > throwForLoadFailureIfNeeded(exceptionState, "Document is already
> detached.");
> > > Is this something required by the spec? I read
> > > https://xhr.spec.whatwg.org/#the-send()-method, and I don't see any
> reference
> > to
> > > this.
> > 
> > Yeah, I couldn't find any description in the spec either.
> > 
> > However, the throwForLoadFailureIfNeeded is needed to pass
> > fast/xmlhttprequest/xmlhttprequest-detached-mixedcontent-no-crash.html. The
> > behavior also matches what Firefox does. In any case, it's needed to keep
the
> > existing behavior.
> 
> What happens if we don't call this? The old code just returned false in this
> case, I thought?

PS9 does this, and
fast/xmlhttprequest/xmlhttprequest-detached-mixedcontent-no-crash.html fails.

[dcheng, 2016-09-17 06:12:54]

OK, I see, this used to error out inside createRequest(), but that doesn't work
because we'll crash before we get there now.

LGTM

[haraken, 2016-09-20 07:12:08]

Description was changed from

==========
Clear LifecycleObserver::m_context when LifecycleObserver::contextDestroyed gets
called

This CL clears LifecycleObserver::m_context when
LifecycleObserver::contextDestroyed gets called.
This means that LifecycleObserver::context() starts returning false after the
context gets destroyed.

The new behavior makes a lot of more sense, but the problem is that some
existing code is assuming that
LifecycleObserver::context() keeps returning the context even after the context
gets destroyed.
This CL fixed the problematic code to make all layout tests pass, but I'm not
sure if I've updated all of the problematic code (because the coverage of the
layout tests is not sufficient).
However, the worst thing that can happen is that code that had been assuming
that LifecycleObserver::context()
returns a context even after the context gets destroyed will stop working (it
may cause a null-deref crash).
I think the risk is low.

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

to

==========
Clear LifecycleObserver::m_context when LifecycleObserver::contextDestroyed gets
called

This CL clears LifecycleObserver::m_context when
LifecycleObserver::contextDestroyed gets called.
This means that LifecycleObserver::context() starts returning false after the
context gets destroyed.

The new behavior makes a lot of more sense, but the problem is that some
existing code is assuming that
LifecycleObserver::context() keeps returning the context even after the context
gets destroyed.
This CL fixed the problematic code to make all layout tests pass, but I'm not
sure if I've updated all of the problematic code
(because the coverage of the layout tests is not sufficient).
However, the worst thing that can happen is that code that had been assuming
that LifecycleObserver::context()
returns a context even after the context gets destroyed will stop working (it
may cause a null-deref crash).
I think the risk is low.

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

[haraken, 2016-09-21 00:55:29]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-09-21 00:56:05]

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-09-21 02:22:57]

Description was changed from

==========
Clear LifecycleObserver::m_context when LifecycleObserver::contextDestroyed gets
called

This CL clears LifecycleObserver::m_context when
LifecycleObserver::contextDestroyed gets called.
This means that LifecycleObserver::context() starts returning false after the
context gets destroyed.

The new behavior makes a lot of more sense, but the problem is that some
existing code is assuming that
LifecycleObserver::context() keeps returning the context even after the context
gets destroyed.
This CL fixed the problematic code to make all layout tests pass, but I'm not
sure if I've updated all of the problematic code
(because the coverage of the layout tests is not sufficient).
However, the worst thing that can happen is that code that had been assuming
that LifecycleObserver::context()
returns a context even after the context gets destroyed will stop working (it
may cause a null-deref crash).
I think the risk is low.

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

to

==========
Clear LifecycleObserver::m_context when LifecycleObserver::contextDestroyed gets
called

This CL clears LifecycleObserver::m_context when
LifecycleObserver::contextDestroyed gets called.
This means that LifecycleObserver::context() starts returning false after the
context gets destroyed.

The new behavior makes a lot of more sense, but the problem is that some
existing code is assuming that
LifecycleObserver::context() keeps returning the context even after the context
gets destroyed.
This CL fixed the problematic code to make all layout tests pass, but I'm not
sure if I've updated all of the problematic code
(because the coverage of the layout tests is not sufficient).
However, the worst thing that can happen is that code that had been assuming
that LifecycleObserver::context()
returns a context even after the context gets destroyed will stop working (it
may cause a null-deref crash).
I think the risk is low.

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

[commit-bot: I haz the power, 2016-09-21 02:22:58]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-09-21 02:25:10]

Description was changed from

==========
Clear LifecycleObserver::m_context when LifecycleObserver::contextDestroyed gets
called

This CL clears LifecycleObserver::m_context when
LifecycleObserver::contextDestroyed gets called.
This means that LifecycleObserver::context() starts returning false after the
context gets destroyed.

The new behavior makes a lot of more sense, but the problem is that some
existing code is assuming that
LifecycleObserver::context() keeps returning the context even after the context
gets destroyed.
This CL fixed the problematic code to make all layout tests pass, but I'm not
sure if I've updated all of the problematic code
(because the coverage of the layout tests is not sufficient).
However, the worst thing that can happen is that code that had been assuming
that LifecycleObserver::context()
returns a context even after the context gets destroyed will stop working (it
may cause a null-deref crash).
I think the risk is low.

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

to

==========
Clear LifecycleObserver::m_context when LifecycleObserver::contextDestroyed gets
called

This CL clears LifecycleObserver::m_context when
LifecycleObserver::contextDestroyed gets called.
This means that LifecycleObserver::context() starts returning false after the
context gets destroyed.

The new behavior makes a lot of more sense, but the problem is that some
existing code is assuming that
LifecycleObserver::context() keeps returning the context even after the context
gets destroyed.
This CL fixed the problematic code to make all layout tests pass, but I'm not
sure if I've updated all of the problematic code
(because the coverage of the layout tests is not sufficient).
However, the worst thing that can happen is that code that had been assuming
that LifecycleObserver::context()
returns a context even after the context gets destroyed will stop working (it
may cause a null-deref crash).
I think the risk is low.

BUG=610176

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

[commit-bot: I haz the power, 2016-09-21 02:25:11]

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

[loyso (OOO), 2016-09-21 04:20:20]

Looks like this CL causes webkit_tests to fail on Linux
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/b...
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/b...
Any ideas?

[haraken, 2016-09-21 04:29:51]

On 2016/09/21 04:20:20, loyso wrote:
> Looks like this CL causes webkit_tests to fail on Linux
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/b...
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/b...
> Any ideas?

Looking

[haraken, 2016-09-21 04:40:47]

On 2016/09/21 04:29:51, haraken wrote:
> On 2016/09/21 04:20:20, loyso wrote:
> > Looks like this CL causes webkit_tests to fail on Linux
> >
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/b...
> >
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/b...
> > Any ideas?
> 
> Looking

Landing a fix: https://codereview.chromium.org/2357833002/

[loyso (OOO), 2016-09-21 05:45:22]

On 2016/09/21 04:40:47, haraken wrote:
> Landing a fix: https://codereview.chromium.org/2357833002/

Thanks for the prompt fix!

[dcheng, 2016-09-22 18:46:29]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/2365463004/ by dcheng@chromium.org.

The reason for reverting is: Speculatively reverting for
https://crbug.com/649272.