Should not call FocusController::setFocusedFrame() for detached frame

third_party/WebKit/Source/core/page/FocusController.cpp

 /*
  * Copyright (C) 2006, 2007 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nuanti Ltd.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 754 matching lines @@
     }
 
     LocalFrame* newFocusedFrame = (frame && frame->isLocalFrame()) ? toLocalFrame(frame) : nullptr;
     if (newFocusedFrame && newFocusedFrame->view()) {
         Document* document = newFocusedFrame->document();
         Element* focusedElement = document ? document->focusedElement() : nullptr;
         if (focusedElement)
             dispatchFocusEvent(*document, *focusedElement);
     }
 
+    // dispatchBlurEvent/dispatchFocusEvent could have changed the focused frame, or
+    // detached the frame.
+    if (newFocusedFrame && !newFocusedFrame->selection().isAvailable())

[yosin_UTC9, 2016/07/21 06:10:10]

It is better to use |newFocusedFrame && newFocusedFrame->view()|,
selection is availability isn't directly related to frame availability.

[kochi, 2016/07/21 06:55:26]

Done.

+        return;
+
     setFocusedFrame(frame, notifyEmbedder);
 }
 
 LocalFrame* FocusController::focusedFrame() const
 {
     // TODO(alexmos): Strengthen this to DCHECK that whoever called this really
     // expected a LocalFrame. Refactor call sites so that the rare cases that
     // need to know about focused RemoteFrames use a separate accessor (to be
     // added).
     if (m_focusedFrame && m_focusedFrame->isRemoteFrame())
@@ skipping 323 matching lines @@
     if (m_isActive == active)
         return;
 
     m_isActive = active;
 
     Frame* frame = focusedOrMainFrame();
     if (frame->isLocalFrame()) {
         // Invalidate all custom scrollbars because they support the CSS
         // window-active attribute. This should be applied to the entire page so
         // we invalidate from the root FrameView instead of just the focused.
-        if (FrameView* view = toLocalFrame(frame)->localFrameRoot()->document()->view())
+        if (FrameView* view = toLocalFrame(frame)->localFrameRoot()->document()->view()) {
             view->invalidateAllCustomScrollbarsOnActiveChanged();
-        toLocalFrame(frame)->selection().pageActivationChanged();
+            toLocalFrame(frame)->selection().pageActivationChanged();

[yosin_UTC9, 2016/07/21 06:10:10]

Can we assume that |localFrameRoot()->view() != nullptr| implies
|frame->selection().isAvailable()|?
It seems this assumption is weak, e.g. |frame| is inside remote frame.
It is better to check frame->selection().isAvailable() instead of
|localFrameRoot()->view() != nullptr|.

[kochi, 2016/07/21 06:55:26]

Agree that |->view()| doesn't imply |selection().isAvailable()| is true.
Check added.

Done.

+        }
     }
 }
 
 static void updateFocusCandidateIfNeeded(WebFocusType type, const FocusCandidate& current, FocusCandidate& candidate, FocusCandidate& closest)
 {
     DCHECK(candidate.visibleNode->isElementNode());
     DCHECK(candidate.visibleNode->layoutObject());
 
     // Ignore iframes that don't have a src attribute
     if (frameOwnerElement(candidate) && (!frameOwnerElement(candidate)->contentFrame() || candidate.rect.isEmpty()))
@@ skipping 177 matching lines @@
     return consumed;
 }
 
 DEFINE_TRACE(FocusController)
 {
     visitor->trace(m_page);
     visitor->trace(m_focusedFrame);
 }
 
 } // namespace blink