Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1202)

Issue 1931793002: Stop using nested message loop for alert() and other JS dialogs (Closed)

Created:
4 years, 7 months ago by Changwan Ryu
Modified:
4 years, 7 months ago
CC:
chromium-apps-reviews_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, extensions-reviews_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Stop using nested message loop for alert() and other JS dialogs alert() needs to block on a single boolean result and is currently using a nested loop to process async messages while waiting for a reply from browser process. The nested loop adds a huge complexity to the subsystem as it requires support for reentrancy and reordering in handling every message. Especially, when ImeThread feature was enabled, it was causing a DCHECK failure and a hang because some messages got unexpectedly reordered. Instead of landing a workaround for ImeThread case, this removes nested message loop use case for RenderFrameImpl. Other changes here includes: 1) Fix ExtensionApiTest not to send async messages and wait for result while alert dialog is showing up. This would hang otherwise. 2) Change UnloadController / FastUnloadController to check whether beforeunload events have already been fired in the case of devtools. We were firing beforeunload events twice when closing devtools incorrectly even before this change, but closing process continued because async messages were allowed. This fixes DevToolsBeforeUnloadTest::TestUndockedDevToolsApplicationClose. 3) For 2), change Browser and BrowserTabStripModel to call non-static member functions. 4) Fix UnloadTest by removing the two second wait limit. Sometimes it took more than 2 seconds just to close the browser, but the test could close the browser because async messages were allowed. Now the test fails if before unload dialog shows up, so we prevent it by running an infinite loop. BUG=604675 Committed: https://crrev.com/6ed4d4373c5ce4743850df50f94b867240472033 Cr-Commit-Position: refs/heads/master@{#394883}

Patch Set 1 #

Patch Set 2 : #

Total comments: 4

Patch Set 3 : removed DoNotNotifyWebKitOfModalLoop and fixed test #

Patch Set 4 : use infinite loop instead of 10 secs #

Total comments: 2

Patch Set 5 : updated comment #

Total comments: 11

Patch Set 6 : updated comments and removed unused declaration #

Unified diffs Side-by-side diffs Delta from patch set Stats (+69 lines, -82 lines) Patch
M chrome/browser/extensions/content_script_apitest.cc View 3 chunks +0 lines, -3 lines 0 comments Download
M chrome/browser/ui/browser.h View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/ui/browser.cc View 1 2 3 4 5 1 chunk +14 lines, -0 lines 0 comments Download
M chrome/browser/ui/browser_tab_strip_model_delegate.cc View 1 chunk +2 lines, -10 lines 0 comments Download
M chrome/browser/ui/fast_unload_controller.h View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/fast_unload_controller.cc View 1 2 3 4 5 2 chunks +7 lines, -2 lines 0 comments Download
M chrome/browser/ui/unload_controller.h View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/unload_controller.cc View 1 2 3 4 5 2 chunks +7 lines, -2 lines 0 comments Download
M chrome/browser/unload_browsertest.cc View 1 2 3 2 chunks +5 lines, -13 lines 0 comments Download
M content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java View 1 2 3 4 5 1 chunk +18 lines, -0 lines 0 comments Download
M content/renderer/render_frame_impl.h View 1 2 3 4 5 1 chunk +0 lines, -3 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 3 chunks +2 lines, -14 lines 0 comments Download
M content/renderer/render_thread_impl.h View 1 2 3 4 5 2 chunks +0 lines, -8 lines 0 comments Download
M content/renderer/render_thread_impl.cc View 1 2 3 4 5 4 chunks +7 lines, -23 lines 0 comments Download

Messages

Total messages: 29 (9 generated)
Changwan Ryu
4 years, 7 months ago (2016-04-28 08:18:44 UTC) #3
Changwan Ryu
On 2016/04/28 08:18:44, Changwan Ryu wrote: Please hold your review. Let me investigate the test ...
4 years, 7 months ago (2016-04-29 00:15:33 UTC) #4
aelias_OOO_until_Jul13
FYI, I've discovered a PPAPI use of nested message loop in https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/pepper/ppb_flash_message_loop_impl.cc&q=willEnterModalLoop&sq=package:chromium&dr=C&l=91 . This code ...
4 years, 7 months ago (2016-04-29 05:50:38 UTC) #5
Changwan Ryu
https://codereview.chromium.org/1931793002/diff/20001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1931793002/diff/20001/content/renderer/render_frame_impl.cc#newcode2134 content/renderer/render_frame_impl.cc:2134: // equivalent of WebView::willEnterModalLoop. In this case it is ...
4 years, 7 months ago (2016-04-29 08:44:13 UTC) #6
Changwan Ryu
lushnikov@ and jeremy@, I found that you have written and reviewed devtools-specific logic that was ...
4 years, 7 months ago (2016-05-02 09:14:22 UTC) #9
lushnikov
Thanks so much! Unload/Fastunload controllers lgtm.
4 years, 7 months ago (2016-05-02 18:17:56 UTC) #10
lushnikov
The unload_browsertest.cc looks good as well. Thank you!
4 years, 7 months ago (2016-05-02 18:20:14 UTC) #11
Changwan Ryu
sievers@chromium.org: Please review changes in content/renderer/ thestig@chromium.org: Please review changes in chrome/browser/
4 years, 7 months ago (2016-05-03 01:23:11 UTC) #13
Lei Zhang
chrome/browser lgtm https://codereview.chromium.org/1931793002/diff/60001/content/renderer/render_frame_impl.h File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1931793002/diff/60001/content/renderer/render_frame_impl.h#newcode858 content/renderer/render_frame_impl.h:858: // Sends a message and runs a ...
4 years, 7 months ago (2016-05-03 18:02:10 UTC) #14
Changwan Ryu
https://codereview.chromium.org/1931793002/diff/60001/content/renderer/render_frame_impl.h File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1931793002/diff/60001/content/renderer/render_frame_impl.h#newcode858 content/renderer/render_frame_impl.h:858: // Sends a message and runs a nested message ...
4 years, 7 months ago (2016-05-10 01:49:22 UTC) #15
Changwan Ryu
On 2016/05/10 01:49:22, Changwan Ryu wrote: > https://codereview.chromium.org/1931793002/diff/60001/content/renderer/render_frame_impl.h > File content/renderer/render_frame_impl.h (right): > > https://codereview.chromium.org/1931793002/diff/60001/content/renderer/render_frame_impl.h#newcode858 ...
4 years, 7 months ago (2016-05-17 09:09:16 UTC) #16
aelias_OOO_until_Jul13
non-OWNER lgtm modulo comment below, still needs sievers@ review for content/renderer. https://codereview.chromium.org/1931793002/diff/80001/content/renderer/render_frame_impl.h File content/renderer/render_frame_impl.h (right): ...
4 years, 7 months ago (2016-05-17 19:35:29 UTC) #17
no sievers
https://codereview.chromium.org/1931793002/diff/80001/content/renderer/render_thread_impl.cc File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1931793002/diff/80001/content/renderer/render_thread_impl.cc#newcode950 content/renderer/render_thread_impl.cc:950: // plugin. The remedy is to pump messages on ...
4 years, 7 months ago (2016-05-18 20:11:24 UTC) #18
Changwan Ryu
https://codereview.chromium.org/1931793002/diff/80001/content/renderer/render_frame_impl.h File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1931793002/diff/80001/content/renderer/render_frame_impl.h#newcode854 content/renderer/render_frame_impl.h:854: bool SendSyncMessage(IPC::SyncMessage* message); On 2016/05/17 19:35:29, aelias wrote: > ...
4 years, 7 months ago (2016-05-19 00:44:56 UTC) #19
Lei Zhang
https://codereview.chromium.org/1931793002/diff/80001/content/renderer/render_thread_impl.cc File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1931793002/diff/80001/content/renderer/render_thread_impl.cc#newcode963 content/renderer/render_thread_impl.cc:963: WebView::willEnterModalLoop(); On 2016/05/19 00:44:56, Changwan Ryu wrote: > On ...
4 years, 7 months ago (2016-05-19 00:50:28 UTC) #20
Changwan Ryu
https://codereview.chromium.org/1931793002/diff/80001/content/renderer/render_thread_impl.cc File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1931793002/diff/80001/content/renderer/render_thread_impl.cc#newcode950 content/renderer/render_thread_impl.cc:950: // plugin. The remedy is to pump messages on ...
4 years, 7 months ago (2016-05-19 01:00:31 UTC) #21
no sievers
On 2016/05/19 01:00:31, Changwan Ryu wrote: > https://codereview.chromium.org/1931793002/diff/80001/content/renderer/render_thread_impl.cc > File content/renderer/render_thread_impl.cc (right): > > https://codereview.chromium.org/1931793002/diff/80001/content/renderer/render_thread_impl.cc#newcode950 ...
4 years, 7 months ago (2016-05-19 18:07:56 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1931793002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1931793002/100001
4 years, 7 months ago (2016-05-19 19:16:12 UTC) #25
commit-bot: I haz the power
Committed patchset #6 (id:100001)
4 years, 7 months ago (2016-05-19 22:04:08 UTC) #27
commit-bot: I haz the power
4 years, 7 months ago (2016-05-19 22:05:30 UTC) #29
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/6ed4d4373c5ce4743850df50f94b867240472033
Cr-Commit-Position: refs/heads/master@{#394883}

Powered by Google App Engine
This is Rietveld 408576698