frame()->host() may be null in ScreenOrientationController::pageVisibilityChanged

frame()->host() may be null in ScreenOrientationController::pageVisibilityChanged

This CL adds a check to isActiveAndVisible() so that
ScreenOrientationController::pageVisibilityChanged don't access frame()->host()
which is already null.

BUG=640719
Committed: https://crrev.com/1014f29772853eec10675e2febb6e0e8561cf440
Cr-Commit-Position: refs/heads/master@{#414301}

[haraken, 2016-08-25 02:21:57]

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

[haraken, 2016-08-25 02:22:31]

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

[haraken, 2016-08-25 02:22:32]

PTAL

[commit-bot: I haz the power, 2016-08-25 02:23:06]

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

[dcheng, 2016-08-25 02:31:42]

On 2016/08/25 02:22:32, haraken wrote:
> PTAL

It's a little surprising that we don't proactively set m_frame to null once the
DOMWindowProperty is detached.

The fix LGTM, but maybe we'd be better off just making this a
ContextLifecycleObserver, since that's the eventual plan.

[haraken, 2016-08-25 03:54:43]

On 2016/08/25 02:31:42, dcheng wrote:
> On 2016/08/25 02:22:32, haraken wrote:
> > PTAL
> 
> It's a little surprising that we don't proactively set m_frame to null once
the
> DOMWindowProperty is detached.

Yeah, this makes sense. I can try that but it would be better to just replace it
with ContextLifecycleObserver.

(Note: The current ContextLifecycleObserver doesn't proactively clear m_context
when the context is destroyed and it seems not easy to change that behavior.)

> 
> The fix LGTM, but maybe we'd be better off just making this a
> ContextLifecycleObserver, since that's the eventual plan.

[haraken, 2016-08-25 03:54:48]

The CQ bit was unchecked by haraken@chromium.org

[haraken, 2016-08-25 03:54:53]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-08-25 03:55:21]

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-08-25 03:59:17]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-25 04:01:06]

Description was changed from

==========
frame()->host() may be null in
ScreenOrientationController::pageVisibilityChanged

This CL adds a check to isActiveAndVisible() so that
ScreenOrientationController::pageVisibilityChanged don't access frame()->host()
which is already null.

BUG=640719
==========

to

==========
frame()->host() may be null in
ScreenOrientationController::pageVisibilityChanged

This CL adds a check to isActiveAndVisible() so that
ScreenOrientationController::pageVisibilityChanged don't access frame()->host()
which is already null.

BUG=640719

Committed: https://crrev.com/1014f29772853eec10675e2febb6e0e8561cf440
Cr-Commit-Position: refs/heads/master@{#414301}
==========

[commit-bot: I haz the power, 2016-08-25 04:01:07]

Patchset 1 (id:??) landed as
https://crrev.com/1014f29772853eec10675e2febb6e0e8561cf440
Cr-Commit-Position: refs/heads/master@{#414301}

[dcheng, 2016-08-25 04:02:51]

On 2016/08/25 03:54:43, haraken wrote:
> On 2016/08/25 02:31:42, dcheng wrote:
> > On 2016/08/25 02:22:32, haraken wrote:
> > > PTAL
> > 
> > It's a little surprising that we don't proactively set m_frame to null once
> the
> > DOMWindowProperty is detached.
> 
> Yeah, this makes sense. I can try that but it would be better to just replace
it
> with ContextLifecycleObserver.
> 
> (Note: The current ContextLifecycleObserver doesn't proactively clear
m_context
> when the context is destroyed and it seems not easy to change that behavior.)
> 
> > 
> > The fix LGTM, but maybe we'd be better off just making this a
> > ContextLifecycleObserver, since that's the eventual plan.

I wonder if it's redundant to check both m_frame and m_client? It seems like
m_client will always go null before m_frame.

[haraken, 2016-08-25 04:48:56]

On 2016/08/25 04:02:51, dcheng wrote:
> On 2016/08/25 03:54:43, haraken wrote:
> > On 2016/08/25 02:31:42, dcheng wrote:
> > > On 2016/08/25 02:22:32, haraken wrote:
> > > > PTAL
> > > 
> > > It's a little surprising that we don't proactively set m_frame to null
once
> > the
> > > DOMWindowProperty is detached.
> > 
> > Yeah, this makes sense. I can try that but it would be better to just
replace
> it
> > with ContextLifecycleObserver.
> > 
> > (Note: The current ContextLifecycleObserver doesn't proactively clear
> m_context
> > when the context is destroyed and it seems not easy to change that
behavior.)
> > 
> > > 
> > > The fix LGTM, but maybe we'd be better off just making this a
> > > ContextLifecycleObserver, since that's the eventual plan.
> 
> I wonder if it's redundant to check both m_frame and m_client? It seems like
> m_client will always go null before m_frame.

You're right. Uploaded a fix: https://codereview.chromium.org/2273193003