Detach SpeechRecognitionController upon page detach.

Detach SpeechRecognitionController upon page detach.

When a page is notified destroyed, it is no longer safe to access
the page's SpeechRecognitionController as its lifetime is that of
the page.

R=haraken
BUG=455857
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190993

[sof, 2015-02-26 14:49:35]

sigbjornf@opera.com changed reviewers:
+ haraken@chromium.org, mkwst@chromium.org, mlamouri@chromium.org

[sof, 2015-02-26 14:49:36]

Please take a look.

Checking with the bots wrt the lifetime observer detachment change, but any
early feedback much appreciated.

[haraken, 2015-02-26 16:29:05]

https://codereview.chromium.org/960223002/diff/40001/Source/core/page/Page.cpp
File Source/core/page/Page.cpp (right):

https://codereview.chromium.org/960223002/diff/40001/Source/core/page/Page.cp...
Source/core/page/Page.cpp:613: Page::detachObservers();

Hmm, I might want to avoid introducing another lifecycle notification (if
possible).

What happens if we do something like:

for (Observer* observer : m_observers) {
  observer->contextDestroyed();
}
for (Observer* observer : m_observers) {
  observer->clearLifecycleContext();
}

in notifyContextDestroyed()?

[sof, 2015-02-26 17:01:22]

https://codereview.chromium.org/960223002/diff/40001/Source/core/page/Page.cpp
File Source/core/page/Page.cpp (right):

https://codereview.chromium.org/960223002/diff/40001/Source/core/page/Page.cp...
Source/core/page/Page.cpp:613: Page::detachObservers();
On 2015/02/26 16:29:05, haraken wrote:
> 
> Hmm, I might want to avoid introducing another lifecycle notification (if
> possible).
> 
> What happens if we do something like:
> 
> for (Observer* observer : m_observers) {
>   observer->contextDestroyed();
> }
> for (Observer* observer : m_observers) {
>   observer->clearLifecycleContext();
> }
> 
> in notifyContextDestroyed()?

Yes, the explicit clearing here is not without problems, I've now realized -- if
we clear the observer references when detaching the page, then observer
destructors won't be able to unobserve their contexts when they're destructing.
With fatal consequences.

So, needs more work & thought. Leaning towards relying on
FrameDestructionObserver for SpeechRecognitionController instead.

[mlamouri (slow - plz ping), 2015-02-26 18:06:51]

On 2015/02/26 at 17:01:22, sigbjornf wrote:
> So, needs more work & thought. Leaning towards relying on
FrameDestructionObserver for SpeechRecognitionController instead.

Wouldn't that require the controller to be per-frame?

[sof, 2015-02-26 22:50:27]

On 2015/02/26 18:06:51, Mounir Lamouri wrote:
> On 2015/02/26 at 17:01:22, sigbjornf wrote:
> > So, needs more work & thought. Leaning towards relying on
> FrameDestructionObserver for SpeechRecognitionController instead.
> 
> Wouldn't that require the controller to be per-frame?

Which again would require browser side changes to move client interfaces around,
much as your work is doing.

A simpler approach is to push it up one level to SpeechRecognition & have it
detach its controller. Latest patchset does that.

[haraken, 2015-02-27 08:23:32]

Hmm, this CL will work but it seems a bit strange that
SpeechRecognitionController is bound to a page but SpeechRecognition is
observing a frame to detach the controller. I think these two should be bound to
the same thing. In other words:

- What makes it hard to bind SpeechRecognitionController to a frame?
- What makes it hard to make SpeechRecognition observe a page?

https://codereview.chromium.org/960223002/diff/60001/Source/modules/speech/Sp...
File Source/modules/speech/SpeechRecognition.cpp (right):

https://codereview.chromium.org/960223002/diff/60001/Source/modules/speech/Sp...
Source/modules/speech/SpeechRecognition.cpp:46: LocalFrame* frame =
document->frame();

Just to confirm: A frame is guaranteed to exist here because
SpeechRecognition::create can be created via JavaScript?

[sof, 2015-02-27 08:32:18]

On 2015/02/27 08:23:32, haraken wrote:
> Hmm, this CL will work but it seems a bit strange that
> SpeechRecognitionController is bound to a page but SpeechRecognition is
> observing a frame to detach the controller. I think these two should be bound
to
> the same thing. In other words:
> 
> - What makes it hard to bind SpeechRecognitionController to a frame?

https://codereview.chromium.org/899853002 is an attempt to do so; I don't have
access to the bug that triggered the revert, but it will require the embedder to
expose its speech recognition client via a frame client interface instead, at
least. I don't want to do that here.

> - What makes it hard to make SpeechRecognition observe a page?
> 

A page is owned by a web view & will be destructed upon close() (non-Oilpan.) We
cannot have a script-exposed object that refer directly to a page.

[haraken, 2015-02-27 08:54:35]

On 2015/02/27 08:32:18, sof wrote:
> On 2015/02/27 08:23:32, haraken wrote:
> > Hmm, this CL will work but it seems a bit strange that
> > SpeechRecognitionController is bound to a page but SpeechRecognition is
> > observing a frame to detach the controller. I think these two should be
bound
> to
> > the same thing. In other words:
> > 
> > - What makes it hard to bind SpeechRecognitionController to a frame?
> 
> https://codereview.chromium.org/899853002 is an attempt to do so; I don't have
> access to the bug that triggered the revert, but it will require the embedder
to
> expose its speech recognition client via a frame client interface instead, at
> least. I don't want to do that here.

Makes sense.

> > - What makes it hard to make SpeechRecognition observe a page?
> > 
> 
> A page is owned by a web view & will be destructed upon close() (non-Oilpan.)
We
> cannot have a script-exposed object that refer directly to a page.

Would you elaborate a bit on this? Even if the page is owned by an embedder, the
observer can get a notification when the page is going to get destructed (via
contextDestroyed), can't it?

[sof, 2015-02-27 09:03:46]

On 2015/02/27 08:54:35, haraken wrote:
> On 2015/02/27 08:32:18, sof wrote:
> > On 2015/02/27 08:23:32, haraken wrote:
> > > Hmm, this CL will work but it seems a bit strange that
> > > SpeechRecognitionController is bound to a page but SpeechRecognition is
> > > observing a frame to detach the controller. I think these two should be
> bound
> > to
> > > the same thing. In other words:
> > > 
> > > - What makes it hard to bind SpeechRecognitionController to a frame?
> > 
> > https://codereview.chromium.org/899853002 is an attempt to do so; I don't
have
> > access to the bug that triggered the revert, but it will require the
embedder
> to
> > expose its speech recognition client via a frame client interface instead,
at
> > least. I don't want to do that here.
> 
> Makes sense.
> 
> > > - What makes it hard to make SpeechRecognition observe a page?
> > > 
> > 
> > A page is owned by a web view & will be destructed upon close()
(non-Oilpan.)
> We
> > cannot have a script-exposed object that refer directly to a page.
> 
> Would you elaborate a bit on this? Even if the page is owned by an embedder,
the
> observer can get a notification when the page is going to get destructed (via
> contextDestroyed), can't it?

Yes, it can & take action just like contextDestroyed() now does here over
FrameDestructionObserver. Is one more preferable to the other?

[haraken, 2015-02-27 09:12:22]

On 2015/02/27 09:03:46, sof wrote:
> On 2015/02/27 08:54:35, haraken wrote:
> > On 2015/02/27 08:32:18, sof wrote:
> > > On 2015/02/27 08:23:32, haraken wrote:
> > > > Hmm, this CL will work but it seems a bit strange that
> > > > SpeechRecognitionController is bound to a page but SpeechRecognition is
> > > > observing a frame to detach the controller. I think these two should be
> > bound
> > > to
> > > > the same thing. In other words:
> > > > 
> > > > - What makes it hard to bind SpeechRecognitionController to a frame?
> > > 
> > > https://codereview.chromium.org/899853002 is an attempt to do so; I don't
> have
> > > access to the bug that triggered the revert, but it will require the
> embedder
> > to
> > > expose its speech recognition client via a frame client interface instead,
> at
> > > least. I don't want to do that here.
> > 
> > Makes sense.
> > 
> > > > - What makes it hard to make SpeechRecognition observe a page?
> > > > 
> > > 
> > > A page is owned by a web view & will be destructed upon close()
> (non-Oilpan.)
> > We
> > > cannot have a script-exposed object that refer directly to a page.
> > 
> > Would you elaborate a bit on this? Even if the page is owned by an embedder,
> the
> > observer can get a notification when the page is going to get destructed
(via
> > contextDestroyed), can't it?
> 
> Yes, it can & take action just like contextDestroyed() now does here over
> FrameDestructionObserver. Is one more preferable to the other?

My point is just that if SpeechRecognitionController's lifetime is bound to a
page, then it would be better to make SpeechRecognition observe the page
(instead of a frame associated with the page) to clear the pointer to the
SpeechRecognitionController.

Your change will be safe because the page will outlive the frame, but it would
be better if we don't put any assumption about the lifetimes of the page and the
frame.

[sof, 2015-02-27 10:07:13]

On 2015/02/27 09:12:22, haraken wrote:
> On 2015/02/27 09:03:46, sof wrote:
> > On 2015/02/27 08:54:35, haraken wrote:
> > > On 2015/02/27 08:32:18, sof wrote:
> > > > On 2015/02/27 08:23:32, haraken wrote:
> > > > > Hmm, this CL will work but it seems a bit strange that
> > > > > SpeechRecognitionController is bound to a page but SpeechRecognition
is
> > > > > observing a frame to detach the controller. I think these two should
be
> > > bound
> > > > to
> > > > > the same thing. In other words:
> > > > > 
> > > > > - What makes it hard to bind SpeechRecognitionController to a frame?
> > > > 
> > > > https://codereview.chromium.org/899853002 is an attempt to do so; I
don't
> > have
> > > > access to the bug that triggered the revert, but it will require the
> > embedder
> > > to
> > > > expose its speech recognition client via a frame client interface
instead,
> > at
> > > > least. I don't want to do that here.
> > > 
> > > Makes sense.
> > > 
> > > > > - What makes it hard to make SpeechRecognition observe a page?
> > > > > 
> > > > 
> > > > A page is owned by a web view & will be destructed upon close()
> > (non-Oilpan.)
> > > We
> > > > cannot have a script-exposed object that refer directly to a page.
> > > 
> > > Would you elaborate a bit on this? Even if the page is owned by an
embedder,
> > the
> > > observer can get a notification when the page is going to get destructed
> (via
> > > contextDestroyed), can't it?
> > 
> > Yes, it can & take action just like contextDestroyed() now does here over
> > FrameDestructionObserver. Is one more preferable to the other?
> 
> My point is just that if SpeechRecognitionController's lifetime is bound to a
> page, then it would be better to make SpeechRecognition observe the page
> (instead of a frame associated with the page) to clear the pointer to the
> SpeechRecognitionController.
> 
> Your change will be safe because the page will outlive the frame, but it would
> be better if we don't put any assumption about the lifetimes of the page and
the
> frame.

It makes sense to keep it symmetric; done.

[sof, 2015-02-27 10:12:21]

https://codereview.chromium.org/960223002/diff/60001/Source/modules/speech/Sp...
File Source/modules/speech/SpeechRecognition.cpp (right):

https://codereview.chromium.org/960223002/diff/60001/Source/modules/speech/Sp...
Source/modules/speech/SpeechRecognition.cpp:46: LocalFrame* frame =
document->frame();
On 2015/02/27 08:23:32, haraken wrote:
> 
> Just to confirm: A frame is guaranteed to exist here because
> SpeechRecognition::create can be created via JavaScript?

Not in the general case, SpeechRecognition was vulnerable to crashing in
document/page/frame detached states.

I've made the constructor throw if it is now in such a detached state.

[haraken, 2015-02-27 10:47:39]

LGTM

https://codereview.chromium.org/960223002/diff/80001/Source/modules/speech/Sp...
File Source/modules/speech/SpeechRecognition.cpp (right):

https://codereview.chromium.org/960223002/diff/80001/Source/modules/speech/Sp...
Source/modules/speech/SpeechRecognition.cpp:44: if (!context) {

By calling ConstructorCallWith=ExecutionContext, can we make sure that the
context exists?

[sof, 2015-02-27 10:50:36]

https://codereview.chromium.org/960223002/diff/80001/Source/modules/speech/Sp...
File Source/modules/speech/SpeechRecognition.cpp (right):

https://codereview.chromium.org/960223002/diff/80001/Source/modules/speech/Sp...
Source/modules/speech/SpeechRecognition.cpp:44: if (!context) {
On 2015/02/27 10:47:39, haraken wrote:
> 
> By calling ConstructorCallWith=ExecutionContext, can we make sure that the
> context exists?

The bindings code calls currentExecutionContext() with no null check after, so I
took no chances. Could I?

[haraken, 2015-02-27 10:59:17]

On 2015/02/27 10:50:36, sof wrote:
>
https://codereview.chromium.org/960223002/diff/80001/Source/modules/speech/Sp...
> File Source/modules/speech/SpeechRecognition.cpp (right):
> 
>
https://codereview.chromium.org/960223002/diff/80001/Source/modules/speech/Sp...
> Source/modules/speech/SpeechRecognition.cpp:44: if (!context) {
> On 2015/02/27 10:47:39, haraken wrote:
> > 
> > By calling ConstructorCallWith=ExecutionContext, can we make sure that the
> > context exists?
> 
> The bindings code calls currentExecutionContext() with no null check after, so
I
> took no chances. Could I?

The fact that JavaScript is running indicates that we have a valid
executionContext. So the return value of currentExecutionContext() should not be
null, I think.

[sof, 2015-02-27 11:21:19]

On 2015/02/27 10:59:17, haraken wrote:
> On 2015/02/27 10:50:36, sof wrote:
> >
>
https://codereview.chromium.org/960223002/diff/80001/Source/modules/speech/Sp...
> > File Source/modules/speech/SpeechRecognition.cpp (right):
> > 
> >
>
https://codereview.chromium.org/960223002/diff/80001/Source/modules/speech/Sp...
> > Source/modules/speech/SpeechRecognition.cpp:44: if (!context) {
> > On 2015/02/27 10:47:39, haraken wrote:
> > > 
> > > By calling ConstructorCallWith=ExecutionContext, can we make sure that the
> > > context exists?
> > 
> > The bindings code calls currentExecutionContext() with no null check after,
so
> I
> > took no chances. Could I?
> 
> The fact that JavaScript is running indicates that we have a valid
> executionContext. So the return value of currentExecutionContext() should not
be
> null, I think.

Thanks, valuable knowledge about what's safe to assume. With that asserted for
instead, we can make use of the controller-detached mode that SpeechRecognition
now supports and allow instances to be created without a controller. i.e., no
exceptions thrown.

[sof, 2015-02-27 11:22:30]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-02-27 11:22:31]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/960223002/#ps120001
(title: "Allow controller-detached creation of SpeechRecognition objects")

[commit-bot: I haz the power, 2015-02-27 11:23:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/960223002/120001

[haraken, 2015-02-27 11:29:54]

https://codereview.chromium.org/960223002/diff/120001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognition.cpp (right):

https://codereview.chromium.org/960223002/diff/120001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.cpp:46: SpeechRecognition*
speechRecognition = new SpeechRecognition(document->page(), context);

I think executionContext must not be null but document->page() can be null.

[sof, 2015-02-27 11:33:42]

https://codereview.chromium.org/960223002/diff/120001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognition.cpp (right):

https://codereview.chromium.org/960223002/diff/120001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.cpp:46: SpeechRecognition*
speechRecognition = new SpeechRecognition(document->page(), context);
On 2015/02/27 11:29:53, haraken wrote:
> 
> I think executionContext must not be null but document->page() can be null.

Yes; the supplement's from() would return a nullptr in that empty page case (=>
no controller.)

[haraken, 2015-02-27 11:40:48]

On 2015/02/27 11:33:42, sof wrote:
>
https://codereview.chromium.org/960223002/diff/120001/Source/modules/speech/S...
> File Source/modules/speech/SpeechRecognition.cpp (right):
> 
>
https://codereview.chromium.org/960223002/diff/120001/Source/modules/speech/S...
> Source/modules/speech/SpeechRecognition.cpp:46: SpeechRecognition*
> speechRecognition = new SpeechRecognition(document->page(), context);
> On 2015/02/27 11:29:53, haraken wrote:
> > 
> > I think executionContext must not be null but document->page() can be null.
> 
> Yes; the supplement's from() would return a nullptr in that empty page case
(=>
> no controller.)

Ah, that sounds good.

[commit-bot: I haz the power, 2015-02-27 12:41:50]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190993