Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2646613002:
ShowCreatedWindow: some speculative fixes for 680876. (Closed)
|
Created:
3 years, 11 months ago by ncarter (slow) Modified:
3 years, 11 months ago Reviewers:
alexmos CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShowCreatedWindow: some speculative fixes for 680876.
Bug 678399 is an android-only crash in WebContentsImpl::ShowCreatedWindow, that
seems to have started with recent changes to CreateNewWindow.
The root cause of this issue is not understood, but one possible
explanation is that the |pending_contents_| list winds up tracking
a dead WebContents. This CL adds some defensive code aimed at
either catching a problem earlier, or papering over it.
BUG=680876
Review-Url: https://codereview.chromium.org/2646613002
Cr-Commit-Position: refs/heads/master@{#444939}
Committed: https://chromium.googlesource.com/chromium/src/+/76ac066a7ca9091e4b45b459e1d46f413614a1e1
Patch Set 1 #Patch Set 2 : Add a CHECK #
Total comments: 6
Patch Set 3 : Switch to DWOC. #
Depends on Patchset: Messages
Total messages: 22 (13 generated)
The CQ bit was checked by nick@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...
nick@chromium.org changed reviewers: + alexmos@chromium.org
alexmos, please review (assuming this compiles and goes green)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM but I think the right bug for this is 680876, not 678399? This seems reasonable to try and track this down. https://codereview.chromium.org/2646613002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2646613002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:2221: return; Would it help at all to add a DumpWithoutCrashing to see if we in fact are ever getting here with a null |rwh|? https://codereview.chromium.org/2646613002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:2224: if (contents && contents->GetMainFrame()->GetRenderWidgetHost() == rwh) { How do you think we could get to the state where these are not equal? The main frame swapping to a new RFH/RVH before we get the show IPC? I thought that shouldn't be possible since the renderer calls show() synchronously after createWindow()?
https://codereview.chromium.org/2646613002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2646613002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:2224: if (contents && contents->GetMainFrame()->GetRenderWidgetHost() == rwh) { On 2017/01/18 23:23:47, alexmos wrote: > How do you think we could get to the state where these are not equal? The main > frame swapping to a new RFH/RVH before we get the show IPC? I thought that > shouldn't be possible since the renderer calls show() synchronously after > createWindow()? Really, I'm grasping at straws here: I don't really expect this case to trigger. But the idea is that the delegate did something screwy with the pending contents; it gets a handle to the WebContents via the WebContentsCreated delegate call, and it goes e.g. here: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... . A delegate-specific behavior is my best explanation for this being android-specific. The crash really isn't happening on other platforms. I don't yet see any smoking guns, but it's at least possible there's something in Android-land that would be queuing up an operation here that would immediately commit a navigation?
https://codereview.chromium.org/2646613002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2646613002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:2224: if (contents && contents->GetMainFrame()->GetRenderWidgetHost() == rwh) { On 2017/01/18 23:59:16, ncarter wrote: > On 2017/01/18 23:23:47, alexmos wrote: > > How do you think we could get to the state where these are not equal? The > main > > frame swapping to a new RFH/RVH before we get the show IPC? I thought that > > shouldn't be possible since the renderer calls show() synchronously after > > createWindow()? > > Really, I'm grasping at straws here: I don't really expect this case to trigger. > But the idea is that the delegate did something screwy with the pending > contents; it gets a handle to the WebContents via the WebContentsCreated > delegate call, and it goes e.g. here: > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > . > > A delegate-specific behavior is my best explanation for this being > android-specific. The crash really isn't happening on other platforms. > > I don't yet see any smoking guns, but it's at least possible there's something > in Android-land that would be queuing up an operation here that would > immediately commit a navigation? Agreed - an Android delegate somehow triggering a commit upon observing WC creation is a reasonable theory. Question is - wouldn't we still want to show it in that case? Presumably, it committed something useful in that new WC?
Description was changed from ========== ShowCreatedWindow: some speculative fixes for 678399. Bug 678399 is an android-only crash in WebContentsImpl::ShowCreatedWindow, that seems to have started with recent changes to CreateNewWindow. The root cause of this issue is not understood, but one possible explanation is that the |pending_contents_| list winds up tracking a dead WebContents. This CL adds some defensive code aimed at either catching a problem earlier, or papering over it. BUG=678399 ========== to ========== ShowCreatedWindow: some speculative fixes for 680876. Bug 678399 is an android-only crash in WebContentsImpl::ShowCreatedWindow, that seems to have started with recent changes to CreateNewWindow. The root cause of this issue is not understood, but one possible explanation is that the |pending_contents_| list winds up tracking a dead WebContents. This CL adds some defensive code aimed at either catching a problem earlier, or papering over it. BUG=680876 ==========
The CQ bit was checked by nick@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...
https://codereview.chromium.org/2646613002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2646613002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:2221: return; On 2017/01/18 23:23:47, alexmos (OOO on 1-20) wrote: > Would it help at all to add a DumpWithoutCrashing to see if we in fact are ever > getting here with a null |rwh|? Thanks for this suggestion. I've added a DumpWithoutCrashing here, but only in the case where contents != nullptr, since I'm hoping we don't crash otherwise. https://codereview.chromium.org/2646613002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:2224: if (contents && contents->GetMainFrame()->GetRenderWidgetHost() == rwh) { On 2017/01/19 01:25:20, alexmos (OOO on 1-20) wrote: > On 2017/01/18 23:59:16, ncarter wrote: > > On 2017/01/18 23:23:47, alexmos wrote: > > > How do you think we could get to the state where these are not equal? The > > main > > > frame swapping to a new RFH/RVH before we get the show IPC? I thought that > > > shouldn't be possible since the renderer calls show() synchronously after > > > createWindow()? > > > > Really, I'm grasping at straws here: I don't really expect this case to > trigger. > > But the idea is that the delegate did something screwy with the pending > > contents; it gets a handle to the WebContents via the WebContentsCreated > > delegate call, and it goes e.g. here: > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > . > > > > A delegate-specific behavior is my best explanation for this being > > android-specific. The crash really isn't happening on other platforms. > > > > I don't yet see any smoking guns, but it's at least possible there's something > > in Android-land that would be queuing up an operation here that would > > immediately commit a navigation? > > Agreed - an Android delegate somehow triggering a commit upon observing WC > creation is a reasonable theory. Question is - wouldn't we still want to show > it in that case? Presumably, it committed something useful in that new WC? I undid the equality check since I agree with you about theoretical risk, and I didn't have a strong explanation for it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by nick@chromium.org
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": 40001, "attempt_start_ts": 1484869847304580,
"parent_rev": "e4f4643a0ddc25991785406d4a64b0e51d743ddc", "commit_rev":
"76ac066a7ca9091e4b45b459e1d46f413614a1e1"}
Message was sent while issue was closed.
Description was changed from ========== ShowCreatedWindow: some speculative fixes for 680876. Bug 678399 is an android-only crash in WebContentsImpl::ShowCreatedWindow, that seems to have started with recent changes to CreateNewWindow. The root cause of this issue is not understood, but one possible explanation is that the |pending_contents_| list winds up tracking a dead WebContents. This CL adds some defensive code aimed at either catching a problem earlier, or papering over it. BUG=680876 ========== to ========== ShowCreatedWindow: some speculative fixes for 680876. Bug 678399 is an android-only crash in WebContentsImpl::ShowCreatedWindow, that seems to have started with recent changes to CreateNewWindow. The root cause of this issue is not understood, but one possible explanation is that the |pending_contents_| list winds up tracking a dead WebContents. This CL adds some defensive code aimed at either catching a problem earlier, or papering over it. BUG=680876 Review-Url: https://codereview.chromium.org/2646613002 Cr-Commit-Position: refs/heads/master@{#444939} Committed: https://chromium.googlesource.com/chromium/src/+/76ac066a7ca9091e4b45b459e1d4... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/76ac066a7ca9091e4b45b459e1d4...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2672933002/ by nick@chromium.org. The reason for reverting is: Reverting speculative instrumentation which is no longer necessary, based on our understanding of the crash.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
