Clear Document::m_frame after dispatching ContextLifecycleObesrver::contextDestroyed

Clear Document::m_frame after dispatching ContextLifecycleObesrver::contextDestroyed

Bluetooth needs to use document->frame() in ContextLifecycleObesrver::contextDestroyed().
Hence this CL moves m_frame=nullptr to after dispatching contextDestroyed().

See https://groups.google.com/a/chromium.org/d/topic/blink-dev/sfpRQc4LB_I/discussion for more motivation of this CL.

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

[haraken, 2016-09-23 06:44:45]

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

[haraken, 2016-09-23 06:44:46]

PTAL

[dcheng, 2016-09-23 06:46:51]

The main thing I'm wondering here... why does the bluetooth code supplement
frame if it needs to be cleaned up when the document is detached?

[haraken, 2016-09-23 06:50:27]

On 2016/09/23 06:46:51, dcheng wrote:
> The main thing I'm wondering here... why does the bluetooth code supplement
> frame if it needs to be cleaned up when the document is detached?

What's your concern? I think it's common to clean up things in
ContextLifecycleObserver::stop().

[dcheng, 2016-09-23 07:17:55]

On 2016/09/23 06:50:27, haraken wrote:
> On 2016/09/23 06:46:51, dcheng wrote:
> > The main thing I'm wondering here... why does the bluetooth code supplement
> > frame if it needs to be cleaned up when the document is detached?
> 
> What's your concern? I think it's common to clean up things in
> ContextLifecycleObserver::stop().

Right, the cleanup is fine. I'm just wondering if it would make more sense for
BluetoothSupplement to be a document supplement in the long run.
Thinking about it more though, I guess BluetoothDevice needs to do its own
cleanup anyway, so this is fine. Maybe onion soup + mojo will let us simplify
some of this as well: in theory, if BluetoothSupplement was just an interface
pointer, we could just close the connection in
ContextLifecycleObserver::contextDestroyed(): then the browser side could handle
any cleanup in its connection error handler.

In any case, current CL lgtm

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

On 2016/09/23 07:17:55, dcheng wrote:
> On 2016/09/23 06:50:27, haraken wrote:
> > On 2016/09/23 06:46:51, dcheng wrote:
> > > The main thing I'm wondering here... why does the bluetooth code
supplement
> > > frame if it needs to be cleaned up when the document is detached?
> > 
> > What's your concern? I think it's common to clean up things in
> > ContextLifecycleObserver::stop().
> 
> Right, the cleanup is fine. I'm just wondering if it would make more sense for
> BluetoothSupplement to be a document supplement in the long run.
> Thinking about it more though, I guess BluetoothDevice needs to do its own
> cleanup anyway, so this is fine. Maybe onion soup + mojo will let us simplify
> some of this as well: in theory, if BluetoothSupplement was just an interface
> pointer, we could just close the connection in
> ContextLifecycleObserver::contextDestroyed(): then the browser side could
handle
> any cleanup in its connection error handler.

I think this makes a lot of more sense.

As I mentioned in https://codereview.chromium.org/1334763002/ and
https://codereview.chromium.org/1611773002/, the current shape of
BluetoothSupplement looks nasty. It's not clear what the lifetime of Bluetooth
is associated with -- frame? document? or anything else? I think that's why
BluetoothDevice is hitting the subtlety of the timing when m_frame is cleared.
We should clean up the lifetime relationship as dcheng@ suggested.

[haraken, 2016-09-23 07:24:15]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-09-23 07:24:26]

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-23 07:33:43]

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

[commit-bot: I haz the power, 2016-09-23 07:33:44]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[haraken, 2016-09-23 07:40:16]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-09-23 07:40:17]

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/2362173002/#ps20001
(title: "temp")

[commit-bot: I haz the power, 2016-09-23 07:40:25]

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

[ortuno, 2016-09-23 09:19:33]

Sorry for the trouble here. There is clearly some code debt that we've been
pushing off until we onion soup this. We are a supplement of the frame because
our client implementation is there:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/ChromeClie...

Once I start Onion-souping this I will ask for feedback to avoid repeating the
mistakes.

[ortuno, 2016-09-23 09:20:42]

btw BluetoothRemoteGattCharacteristic follows a similar pattern so we might have
to fix that one as well.

[haraken, 2016-09-23 10:05:03]

On 2016/09/23 09:20:42, ortuno wrote:
> btw BluetoothRemoteGattCharacteristic follows a similar pattern so we might
have
> to fix that one as well.

The design pattern is not nice, but in terms of lifetime this CL is sufficient
to make BluetoothRemoteGattCharacteristic work correctly, right?

[commit-bot: I haz the power, 2016-09-23 11:26:03]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-23 11:29:52]

Description was changed from

==========
Clear Document::m_frame after dispatching
ContextLifecycleObesrver::contextDestroyed

Bluetooth needs to use document->frame() in
ContextLifecycleObesrver::contextDestroyed().
Hence this CL moves m_frame=nullptr to after dispatching contextDestroyed().

See
https://groups.google.com/a/chromium.org/d/topic/blink-dev/sfpRQc4LB_I/discus...
for more motivation of this CL.

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

to

==========
Clear Document::m_frame after dispatching
ContextLifecycleObesrver::contextDestroyed

Bluetooth needs to use document->frame() in
ContextLifecycleObesrver::contextDestroyed().
Hence this CL moves m_frame=nullptr to after dispatching contextDestroyed().

See
https://groups.google.com/a/chromium.org/d/topic/blink-dev/sfpRQc4LB_I/discus...
for more motivation of this CL.

BUG=610176

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

[commit-bot: I haz the power, 2016-09-23 11:29:54]

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