Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2519333007:
<webview> Fix crash when closing chrome://chrome-signin (Closed)
Description<webview> Fix crash when closing chrome://chrome-signin
This fixes a browser crash with OOPIF-based webviews. When two instances
of chrome://chrome-signin are opened in browser tabs, closing either one
of them leads to a crash.
The embedding WebContentsImpl in its destructor will attempt to update
screen rects for child WebContentsImpl. The children will fail to locate
their parent due to their node Id not being kInvalid despite the node no
longer existing (destroyed earlier in the parent WebContentsImpl dtor).
~ No longer notify children about screen rect changes when being
destroyed.
+ Add regression test.
BUG=667708
Committed: https://crrev.com/9f43b0110e3e4b47064b067fecc30ca2ce2c193e
Cr-Commit-Position: refs/heads/master@{#434570}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Only run test on Win/Max/Linux (Chromeos does not implement chrome://chrome-signin) #Patch Set 3 : Address creis@ comment. #Patch Set 4 : OS_LINUX always true when OS_CHROMEOS, excluse OS_CHROMEOS #
Messages
Total messages: 26 (19 generated)
The CQ bit was checked by avallee@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...
avallee@chromium.org changed reviewers: + creis@chromium.org, nasko@chromium.org, wjmaclean@chromium.org
creis@chromium.org or nasko@chromium.org: content/ wjmaclean@chromium.org: chrome/
Thanks! content/ LGTM. (I'll defer to wjmaclean on the test details.) https://codereview.chromium.org/2519333007/diff/1/chrome/browser/apps/guest_v... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/2519333007/diff/1/chrome/browser/apps/guest_v... chrome/browser/apps/guest_view/web_view_browsertest.cc:3865: class ChromeSignInWebViewTest : public WebViewTestBase { Maybe add a comment that this runs with the OOPIF version of WebViews? It's easy to miss in the details below.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by avallee@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 checked by avallee@chromium.org to run a CQ dry run
PTAL. https://codereview.chromium.org/2519333007/diff/1/chrome/browser/apps/guest_v... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/2519333007/diff/1/chrome/browser/apps/guest_v... chrome/browser/apps/guest_view/web_view_browsertest.cc:3865: class ChromeSignInWebViewTest : public WebViewTestBase { On 2016/11/23 23:45:24, Charlie Reis (slow) wrote: > Maybe add a comment that this runs with the OOPIF version of WebViews? It's > easy to miss in the details below. Done.
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_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by avallee@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_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
The CQ bit was checked by avallee@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2519333007/#ps60001 (title: "OS_LINUX always true when OS_CHROMEOS, excluse OS_CHROMEOS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480138273608680,
"parent_rev": "ba6829b80de7de1d3d5790c9468ea257a355d434", "commit_rev":
"8dca4f48031610b1364a02ea5c7e03d16ec430d5"}
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== <webview> Fix crash when closing chrome://chrome-signin This fixes a browser crash with OOPIF-based webviews. When two instances of chrome://chrome-signin are opened in browser tabs, closing either one of them leads to a crash. The embedding WebContentsImpl in its destructor will attempt to update screen rects for child WebContentsImpl. The children will fail to locate their parent due to their node Id not being kInvalid despite the node no longer existing (destroyed earlier in the parent WebContentsImpl dtor). ~ No longer notify children about screen rect changes when being destroyed. + Add regression test. BUG=667708 ========== to ========== <webview> Fix crash when closing chrome://chrome-signin This fixes a browser crash with OOPIF-based webviews. When two instances of chrome://chrome-signin are opened in browser tabs, closing either one of them leads to a crash. The embedding WebContentsImpl in its destructor will attempt to update screen rects for child WebContentsImpl. The children will fail to locate their parent due to their node Id not being kInvalid despite the node no longer existing (destroyed earlier in the parent WebContentsImpl dtor). ~ No longer notify children about screen rect changes when being destroyed. + Add regression test. BUG=667708 Committed: https://crrev.com/9f43b0110e3e4b47064b067fecc30ca2ce2c193e Cr-Commit-Position: refs/heads/master@{#434570} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9f43b0110e3e4b47064b067fecc30ca2ce2c193e Cr-Commit-Position: refs/heads/master@{#434570} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
