Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2153063003:
Should not call FocusController::setFocusedFrame() for detached frame (Closed)
DescriptionShould not call FocusController::setFocusedFrame() for detached frame
In case frame is detached, do not call setFocusedFrame() as it can't be focused.
BUG=626750
TEST=LayoutTests/fast/dom/Selection/selection-crash.html
Committed: https://crrev.com/f0bde6849797c7856ec8c4edadf3b4572c3261f5
Cr-Commit-Position: refs/heads/master@{#407019}
Patch Set 1 #
Total comments: 4
Patch Set 2 : update for comments #Patch Set 3 : Proposed fix #3 #
Total comments: 16
Patch Set 4 : fix for comment #20 #
Total comments: 3
Patch Set 5 : fix for comment #26 #
Messages
Total messages: 44 (26 generated)
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Should not call FocusController::setFocusedFrame() for detached frame In case frame is detached, do not call setFocusedFrame() as it can't be focused. BUG=626750 ========== to ========== Should not call FocusController::setFocusedFrame() for detached frame In case frame is detached, do not call setFocusedFrame() as it can't be focused. BUG=626750 ==========
kochi@chromium.org changed reviewers: + yosin@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
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...)
https://codereview.chromium.org/2153063003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html (right): https://codereview.chromium.org/2153063003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html:10: test(() => {}, "test did not crash."); We should have an assertion to check focused element. I'm not sure when "focus" event dispatch, before/after "load"? I think it is better to use |async_test()| to dispatch "focus" event explicitly rather than depending on "autofocus". https://codereview.chromium.org/2153063003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2153063003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/page/FocusController.cpp:776: setFocusedFrame(frame, notifyEmbedder); I think we should check |LocalFrame::selection().isAvailable())| in |FocusController::setFocusedFrame()| rather than here. |dispatchEvent(blur)| at L737 can destroy |newFrame|. 720 void FocusController::setFocusedFrame(Frame* frame, bool notifyEmbedder) ... 734 // Now that the frame is updated, fire events and update the selection focused states of both frames. 735 if (oldFrame && oldFrame->view()) { 736 oldFrame->selection().setFocused(false); 737 oldFrame->domWindow()->dispatchEvent(Event::create(EventTypeNames::blur)); 738 } 739 740 if (newFrame && newFrame->view() && isFocused()) { 741 newFrame->selection().setFocused(true); 742 newFrame->domWindow()->dispatchEvent(Event::create(EventTypeNames::focus)); 743 }
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Looks like the proposed fix broke site-per-process browser tests. Let me take further look.
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2153063003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html (right): https://codereview.chromium.org/2153063003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html:10: test(() => {}, "test did not crash."); On 2016/07/19 01:44:46, Yosi_UTC9 wrote: > We should have an assertion to check focused element. What is the intention? This focus/blur(focusout) evnet handler will be called during the focus change, not after the all focus changes, including ones caused by event handlers. This test intentionally change focus during focus handler, so testing the result makes little sense here. > I'm not sure when "focus" event dispatch, before/after "load"? > I think it is better to use |async_test()| to dispatch "focus" event explicitly > rather than depending on "autofocus". Changed not to depend on autofocus. Initially I used autofocus because the original reproduction case used, but button.focus() should work as well. https://codereview.chromium.org/2153063003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2153063003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/page/FocusController.cpp:776: setFocusedFrame(frame, notifyEmbedder); On 2016/07/19 01:44:46, Yosi_UTC9 wrote: > I think we should check |LocalFrame::selection().isAvailable())| in > |FocusController::setFocusedFrame()| rather than here. > > |dispatchEvent(blur)| at L737 can destroy |newFrame|. > > 720 void FocusController::setFocusedFrame(Frame* frame, bool notifyEmbedder) > ... > 734 // Now that the frame is updated, fire events and update the selection > focused states of both frames. > 735 if (oldFrame && oldFrame->view()) { > 736 oldFrame->selection().setFocused(false); > 737 > oldFrame->domWindow()->dispatchEvent(Event::create(EventTypeNames::blur)); > 738 } > 739 > 740 if (newFrame && newFrame->view() && isFocused()) { > 741 newFrame->selection().setFocused(true); > 742 > newFrame->domWindow()->dispatchEvent(Event::create(EventTypeNames::focus)); > 743 } The original test case fails after dispatching events in this function (line 764 and 772). Let me try to fix the failing case first.
Ping, could you take a look?
Please add TEST=LayoutTests/fast/dom/Selection/selection-crash.html in description. https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html (right): https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html:11: }, false); nit: Since default value of second parameter is |false|, we don't need to explicitly pass it. https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html:16: assert_true(true, "Test did not crash."); We should check value of |document.activeElement| rather than |assert_true(ture, ...)| https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html:17: }), false); nit: Since default value of second parameter is |false|, we don't need to explicitly pass it. https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html:19: window.addEventListener('load', () => { button.focus(); }, false); nit: We can write |window.addEventListener('load', () => button.focus());| nit: Since default value of second parameter is |false|, we don't need to explicitly pass it. https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html:20: }, "focusing on detached frame should not crash"); nit: use single quote since other parts in script use single quote. https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:777: if (newFocusedFrame && !newFocusedFrame->selection().isAvailable()) It is better to use |newFocusedFrame && newFocusedFrame->view()|, selection is availability isn't directly related to frame availability. https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:1125: toLocalFrame(frame)->selection().pageActivationChanged(); 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|.
Description was changed from ========== Should not call FocusController::setFocusedFrame() for detached frame In case frame is detached, do not call setFocusedFrame() as it can't be focused. BUG=626750 ========== to ========== Should not call FocusController::setFocusedFrame() for detached frame In case frame is detached, do not call setFocusedFrame() as it can't be focused. BUG=626750 TEST=LayoutTests/fast/dom/Selection/selection-crash.html ==========
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL (could you answer my question why document.activeElement check makes sense?) https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html (right): https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html:11: }, false); On 2016/07/21 06:10:10, Yosi_UTC9 wrote: > nit: Since default value of second parameter is |false|, we don't need to > explicitly pass it. Done. https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html:16: assert_true(true, "Test did not crash."); On 2016/07/21 06:10:10, Yosi_UTC9 wrote: > We should check value of |document.activeElement| rather than |assert_true(ture, > ...)| What is the intention of wanting to know the then-active focused element? Immediately after document.open(), document.activeElement will always be null? https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html:17: }), false); On 2016/07/21 06:10:10, Yosi_UTC9 wrote: > nit: Since default value of second parameter is |false|, we don't need to > explicitly pass it. Done. https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html:19: window.addEventListener('load', () => { button.focus(); }, false); On 2016/07/21 06:10:10, Yosi_UTC9 wrote: > nit: We can write |window.addEventListener('load', () => button.focus());| > nit: Since default value of second parameter is |false|, we don't need to > explicitly pass it. Done. https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html:20: }, "focusing on detached frame should not crash"); On 2016/07/21 06:10:10, Yosi_UTC9 wrote: > nit: use single quote since other parts in script use single quote. Done. https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:777: if (newFocusedFrame && !newFocusedFrame->selection().isAvailable()) On 2016/07/21 06:10:10, Yosi_UTC9 wrote: > It is better to use |newFocusedFrame && newFocusedFrame->view()|, > selection is availability isn't directly related to frame availability. Done. https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:1125: toLocalFrame(frame)->selection().pageActivationChanged(); On 2016/07/21 06:10:10, Yosi_UTC9 wrote: > 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|. Agree that |->view()| doesn't imply |selection().isAvailable()| is true. Check added. Done.
https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html (right): https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html:16: assert_true(true, "Test did not crash."); On 2016/07/21 at 06:55:26, kochi wrote: > On 2016/07/21 06:10:10, Yosi_UTC9 wrote: > > We should check value of |document.activeElement| rather than |assert_true(ture, > > ...)| > > What is the intention of wanting to know the then-active focused element? > Immediately after document.open(), document.activeElement will always be null? Checking |document.activeElement| leads us |FocusController| does right thing after |document.open()|. No crash is too weak to check regression. We want to known |m_focusedFrame| when setting focus frame is aborted. https://codereview.chromium.org/2153063003/diff/50001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2153063003/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:1126: if (localFrame->selection().isAvailable()) This check should be parallel of |localFrame->localFrameRoot()->document()->view()| as original code and to support |frame| in remote frame.
https://codereview.chromium.org/2153063003/diff/50001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2153063003/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:1126: if (localFrame->selection().isAvailable()) On 2016/07/21 at 07:22:36, Yosi_UTC9 wrote: > This check should be parallel of |localFrame->localFrameRoot()->document()->view()| as original code and to support |frame| in remote frame. BTW, do we really need to change this for crbug.com/626750? Does attached layout test covert this change?
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html (right): https://codereview.chromium.org/2153063003/diff/30003/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/Selection/selection-crash.html:16: assert_true(true, "Test did not crash."); On 2016/07/21 07:22:36, Yosi_UTC9 wrote: > On 2016/07/21 at 06:55:26, kochi wrote: > > On 2016/07/21 06:10:10, Yosi_UTC9 wrote: > > > We should check value of |document.activeElement| rather than > |assert_true(ture, > > > ...)| > > > > What is the intention of wanting to know the then-active focused element? > > Immediately after document.open(), document.activeElement will always be null? > > Checking |document.activeElement| leads us |FocusController| does right thing > after > |document.open()|. > > No crash is too weak to check regression. We want to known |m_focusedFrame| when > setting focus frame is aborted. Originally I thought that this test is for reproducing the crash, and not for testing whether document.open() clears .activeElement or not, and it should be in another layout test, if necessary. Anyway, I found that onfocus handler finishes later than onblur handler, so I added comment about the order of occurrence and changed to use step_func_done for onfocus handler. Added activeElement check next to |iframe.contentWindow.focus();| line, which should make sense. https://codereview.chromium.org/2153063003/diff/50001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2153063003/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:1126: if (localFrame->selection().isAvailable()) On 2016/07/21 07:30:28, Yosi_UTC9 wrote: > On 2016/07/21 at 07:22:36, Yosi_UTC9 wrote: > > This check should be parallel of > |localFrame->localFrameRoot()->document()->view()| as original code and to > support |frame| in remote frame. > > BTW, do we really need to change this for crbug.com/626750? > Does attached layout test covert this change? Originally this was added for the case that appeared in clusterfuzz's backtrace, which calls blink::WebViewImpl::setIsActive() (which calls this function) directly from IPC message pump, that could bypass the code change in focusDocumentView(). But as far as I tested, neither of the original fuzzer test case nor the layout test crashed without this chunk. So I think the IPC happens after setFocusedFrame() on the detached frame. Ideally we should check all the callsites of setFocusedFrame(), but will do in later CL if necessary. Let's fix the failing case for clusterfuzz.
lgtm
The CQ bit was unchecked by kochi@chromium.org
The CQ bit was checked by kochi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
kochi@chromium.org changed reviewers: + tkent@chromium.org
tkent-san, could you review this for OWNERS?
lgtm
The CQ bit was checked by kochi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Should not call FocusController::setFocusedFrame() for detached frame In case frame is detached, do not call setFocusedFrame() as it can't be focused. BUG=626750 TEST=LayoutTests/fast/dom/Selection/selection-crash.html ========== to ========== Should not call FocusController::setFocusedFrame() for detached frame In case frame is detached, do not call setFocusedFrame() as it can't be focused. BUG=626750 TEST=LayoutTests/fast/dom/Selection/selection-crash.html ==========
Message was sent while issue was closed.
Committed patchset #5 (id:70001)
Message was sent while issue was closed.
Description was changed from ========== Should not call FocusController::setFocusedFrame() for detached frame In case frame is detached, do not call setFocusedFrame() as it can't be focused. BUG=626750 TEST=LayoutTests/fast/dom/Selection/selection-crash.html ========== to ========== Should not call FocusController::setFocusedFrame() for detached frame In case frame is detached, do not call setFocusedFrame() as it can't be focused. BUG=626750 TEST=LayoutTests/fast/dom/Selection/selection-crash.html Committed: https://crrev.com/f0bde6849797c7856ec8c4edadf3b4572c3261f5 Cr-Commit-Position: refs/heads/master@{#407019} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f0bde6849797c7856ec8c4edadf3b4572c3261f5 Cr-Commit-Position: refs/heads/master@{#407019} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
