Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1432143002:
Track where WebContents are created in order to better understand issue. (Closed)
|
Created:
5 years, 1 month ago by hcarmona Modified:
5 years ago CC:
chromium-reviews, droger+watchlist_chromium.org, blundell+watchlist_chromium.org, nasko+codewatch_chromium.org, dcheng, nyquist+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, miu+watch_chromium.org, extensions-reviews_chromium.org, maniscalco+watch-blimp_chromium.org, Matt Giuca, lcwu+watch_chromium.org, sdefresne+watchlist_chromium.org, marcinjb+watch-blimp_chromium.org, darin-cc_chromium.org, halliwell+watch_chromium.org, devtools-reviews_chromium.org, chromium-apps-reviews_chromium.org, android-webview-reviews_chromium.org, mlamouri+watch-content_chromium.org, jennb, creis+watch_chromium.org, tapted, Peter Beverloo, jianli, oshima+watch_chromium.org, gunsch+watch_chromium.org, mkwst+moarreviews-shell_chromium.org, jochen+watch_chromium.org, sriramsr+watch-blimp_chromium.org, tfarina, Dmitry Titov, davemoore+watch_chromium.org, dtrainor+watch-blimp_chromium.org, pfeldman Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTrack where WebContents are created in order to better understand issue.
This is a temporary change to track down a beta blocking crash.
This CL doesn't fix the crash, but will give us more information.
The crash is happening when we attempt to show a modal dialog on some
unknown WebContents that doesn't have a WebModalDialogManager. Not all
WebContents are expecting to show modal dialogs and the creator of the
WebModalDialog doesn't control what WebContents they will get at
runtime. So, because of this loose coupling, we don't know which
WebContents is triggering the crash.
By adding this trace we expect to learn where the WebContents are
created in order to make sure they have a WebModalDialogManager.
BUG=538612
Committed: https://crrev.com/e1158d6a42903de1664c12bc91ba056ecb716bb6
Cr-Commit-Position: refs/heads/master@{#360112}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Feedback #Patch Set 3 : StackTrace #
Total comments: 3
Patch Set 4 : Save the Delegate #
Total comments: 3
Patch Set 5 : Same as Patch Set 3 #
Messages
Total messages: 42 (15 generated)
hcarmona@chromium.org changed reviewers: + wittman@chromium.org
Can you provide some guidance on this? I've started what you mentioned on the bug, but this does somethings I'm not 100% sure on. And trybots are upset that I've changed the DEPS for constrained_window_views.
halliwell@chromium.org changed reviewers: + halliwell@chromium.org
https://codereview.chromium.org/1432143002/diff/1/chromecast/browser/cast_con... File chromecast/browser/cast_content_window.cc (right): https://codereview.chromium.org/1432143002/diff/1/chromecast/browser/cast_con... chromecast/browser/cast_content_window.cc:118: WebContentsSource::CreateForWebContentsAndLocation(web_contents, FROM_HERE); Think you can revert this file, your crash isn't in Chromecast ;)
On 2015/11/11 04:39:49, Hector Carmona wrote: > Can you provide some guidance on this? I've started what you mentioned > on the bug, but this does somethings I'm not 100% sure on. And trybots > are upset that I've changed the DEPS for constrained_window_views. Beyond the specific comments, this generally looks fine to me. I assume you've verified that you get correct location information in CreateWebModalDialogViews()? It looks to me like the gn bots just want a dependency declaration on //content/public/browser:browser_sources. You'll probably have an easier review if you stress this is a temporary change to track down a beta-blocker crasher. I'd recommend sky, jam, and rdevlin.cronin as reviewers. https://codereview.chromium.org/1432143002/diff/1/android_webview/lib/aw_brow... File android_webview/lib/aw_browser_dependency_factory_impl.cc (right): https://codereview.chromium.org/1432143002/diff/1/android_webview/lib/aw_brow... android_webview/lib/aw_browser_dependency_factory_impl.cc:42: WebContentsSource::CreateForWebContentsAndLocation(web_contents, FROM_HERE); Web contents modal dialogs aren't supported on Android, so we don't need this one. (+ three other cases) https://codereview.chromium.org/1432143002/diff/1/blimp/engine/browser/blimp_... File blimp/engine/browser/blimp_engine_session.cc (right): https://codereview.chromium.org/1432143002/diff/1/blimp/engine/browser/blimp_... blimp/engine/browser/blimp_engine_session.cc:97: WebContentsSource::CreateForWebContentsAndLocation(web_contents.get(), I don't think blimp is used on desktop, so this probably can also be reverted. https://codereview.chromium.org/1432143002/diff/1/components/constrained_wind... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/1432143002/diff/1/components/constrained_wind... components/constrained_window/constrained_window_views.cc:156: base::debug::Alias(&created_location); needs an #include
Description was changed from ========== Track where WebContents are created in order to better understand issue. This CL doesn't fix the crash, but will give us more information. BUG=538612 ========== to ========== Track where WebContents are created in order to better understand issue. This is a temporary change to track down a beta blocking crash. This CL doesn't fix the crash, but will give us more information. BUG=538612 ==========
Patchset #2 (id:20001) has been deleted
On 2015/11/11 17:15:28, Mike Wittman wrote: > On 2015/11/11 04:39:49, Hector Carmona wrote: > > Can you provide some guidance on this? I've started what you mentioned > > on the bug, but this does somethings I'm not 100% sure on. And trybots > > are upset that I've changed the DEPS for constrained_window_views. > > Beyond the specific comments, this generally looks fine to me. I assume you've > verified that you get correct location information in > CreateWebModalDialogViews()? Yes, used the debugger to verify the web contents owner of print preview. > It looks to me like the gn bots just want a dependency declaration on > //content/public/browser:browser_sources. Fixed gn dependency issues > You'll probably have an easier review if you stress this is a temporary change > to track down a beta-blocker crasher. I'd recommend sky, jam, and rdevlin.cronin > as reviewers. I've updated the description to mention this is temporary. https://codereview.chromium.org/1432143002/diff/1/android_webview/lib/aw_brow... File android_webview/lib/aw_browser_dependency_factory_impl.cc (right): https://codereview.chromium.org/1432143002/diff/1/android_webview/lib/aw_brow... android_webview/lib/aw_browser_dependency_factory_impl.cc:42: WebContentsSource::CreateForWebContentsAndLocation(web_contents, FROM_HERE); On 2015/11/11 17:15:28, Mike Wittman wrote: > Web contents modal dialogs aren't supported on Android, so we don't need this > one. (+ three other cases) Done. https://codereview.chromium.org/1432143002/diff/1/blimp/engine/browser/blimp_... File blimp/engine/browser/blimp_engine_session.cc (right): https://codereview.chromium.org/1432143002/diff/1/blimp/engine/browser/blimp_... blimp/engine/browser/blimp_engine_session.cc:97: WebContentsSource::CreateForWebContentsAndLocation(web_contents.get(), On 2015/11/11 17:15:28, Mike Wittman wrote: > I don't think blimp is used on desktop, so this probably can also be reverted. Done. https://codereview.chromium.org/1432143002/diff/1/chromecast/browser/cast_con... File chromecast/browser/cast_content_window.cc (right): https://codereview.chromium.org/1432143002/diff/1/chromecast/browser/cast_con... chromecast/browser/cast_content_window.cc:118: WebContentsSource::CreateForWebContentsAndLocation(web_contents, FROM_HERE); On 2015/11/11 14:14:31, halliwell wrote: > Think you can revert this file, your crash isn't in Chromecast ;) Done. https://codereview.chromium.org/1432143002/diff/1/components/constrained_wind... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/1432143002/diff/1/components/constrained_wind... components/constrained_window/constrained_window_views.cc:156: base::debug::Alias(&created_location); On 2015/11/11 17:15:28, Mike Wittman wrote: > needs an #include Done.
hcarmona@chromium.org changed reviewers: + jam@chromium.org, rdevlin.cronin@chromium.org, sky@chromium.org
hcarmona@chromium.org changed required reviewers: + jam@chromium.org, rdevlin.cronin@chromium.org, sky@chromium.org
sky@, jam@, Devlin, PTAL, this is a temporary change to get information about a beta blocking crash. Thanks!
On 2015/11/11 20:00:04, Hector Carmona wrote: > sky@, jam@, Devlin, PTAL, this is a temporary change to get information about a > beta blocking crash. > > Thanks! are there simpler ways of doing this, i.e. storing a base::debug::StackTrace in WebContentsImpl's constructor?
also, to save reviewers the time from having to read the bug in detail. can you give a summary of what the crash is? and how this data will be used to find it?
On 2015/11/11 23:11:08, jam wrote: > On 2015/11/11 20:00:04, Hector Carmona wrote: > > sky@, jam@, Devlin, PTAL, this is a temporary change to get information about > a > > beta blocking crash. > > > > Thanks! > > are there simpler ways of doing this, i.e. storing a base::debug::StackTrace in > WebContentsImpl's constructor? I'll take a look at using base::debug::StackTrace in WebContentsImpl's ctor. On 2015/11/11 23:11:46, jam wrote: > also, to save reviewers the time from having to read the bug in detail. > > can you give a summary of what the crash is? and how this data will be used to > find it? The crash is happening when we attempt to show a modal dialog on some unknown WebContents that doesn't have a WebModalDialogManager. Not all WebContents are expecting to show modal dialogs and the creator of the WebModalDialog doesn't control what WebContents they will get at runtime. So, because of this loose coupling, we don't know which WebContents is triggering the crash. By adding this trace we will know where the WebContents are created in order to make sure they have a WebModalDialogManager.
I've updated this CL to use base::debug::StackTrace. I took a look using the debugger to verify that the right info is there. Is there anything that I need to do other than base::debug::Alias(...) to make sure the StackTrace will be available in a minidump?
On 2015/11/12 21:34:04, Hector Carmona wrote: > I've updated this CL to use base::debug::StackTrace. I took a look using > the debugger to verify that the right info is there. > > Is there anything that I need to do other than base::debug::Alias(...) > to make sure the StackTrace will be available in a minidump? Friendly ping on this CL :-)
On 2015/11/16 18:18:14, Hector Carmona wrote: > On 2015/11/12 21:34:04, Hector Carmona wrote: > > I've updated this CL to use base::debug::StackTrace. I took a look using > > the debugger to verify that the right info is there. > > > > Is there anything that I need to do other than base::debug::Alias(...) > > to make sure the StackTrace will be available in a minidump? > > Friendly ping on this CL :-) Since all the extensions/ code is gone from this, I don't think my lg (or that of many people on the reviewers list) is needed. But I'll wish you good luck. :)
Description was changed from ========== Track where WebContents are created in order to better understand issue. This is a temporary change to track down a beta blocking crash. This CL doesn't fix the crash, but will give us more information. BUG=538612 ========== to ========== Track where WebContents are created in order to better understand issue. This is a temporary change to track down a beta blocking crash. This CL doesn't fix the crash, but will give us more information. The crash is happening when we attempt to show a modal dialog on some unknown WebContents that doesn't have a WebModalDialogManager. Not all WebContents are expecting to show modal dialogs and the creator of the WebModalDialog doesn't control what WebContents they will get at runtime. So, because of this loose coupling, we don't know which WebContents is triggering the crash. By adding this trace we expect to learn where the WebContents are created in order to make sure they have a WebModalDialogManager. BUG=538612 ==========
hcarmona@chromium.org changed reviewers: - halliwell@chromium.org, rdevlin.cronin@chromium.org
hcarmona@chromium.org changed required reviewers: - rdevlin.cronin@chromium.org
nick@chromium.org changed reviewers: + nick@chromium.org
I have an idea to make this much simpler. https://codereview.chromium.org/1432143002/diff/60001/components/constrained_... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/1432143002/diff/60001/components/constrained_... components/constrained_window/constrained_window_views.cc:152: base::debug::StackTrace contents_stack_trace = web_contents->stack_trace(); Have you tried just aliasing the value of web_contents->GetDelegate()? That should tell you (say, by looking at the vtable __vfptr under VisualStudio) which subclass of content::WebContentsDelegate this WebContents was created for. These are the subclasses: https://code.google.com/p/chromium/codesearch#search/&q=%22public%20content::...
hcarmona@chromium.org changed reviewers: + ncarter@chromium.org - jam@chromium.org, nick@chromium.org, sky@chromium.org
hcarmona@chromium.org changed required reviewers: + wittman@chromium.org - jam@chromium.org, sky@chromium.org
I've removed other reviewers and set Mike as required. https://codereview.chromium.org/1432143002/diff/60001/components/constrained_... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/1432143002/diff/60001/components/constrained_... components/constrained_window/constrained_window_views.cc:152: base::debug::StackTrace contents_stack_trace = web_contents->stack_trace(); On 2015/11/16 23:54:45, ncarter wrote: > Have you tried just aliasing the value of web_contents->GetDelegate()? > > That should tell you (say, by looking at the vtable __vfptr under VisualStudio) > which subclass of content::WebContentsDelegate this WebContents was created for. > > These are the subclasses: > https://code.google.com/p/chromium/codesearch#search/&q=%22public%20content::... > This should work if the delegates always have their web contents created in the same way. I got Browser as the delegate when I tried this out with the print preview dialog on a regular window. Does that seem right?
https://codereview.chromium.org/1432143002/diff/60001/components/constrained_... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/1432143002/diff/60001/components/constrained_... components/constrained_window/constrained_window_views.cc:152: base::debug::StackTrace contents_stack_trace = web_contents->stack_trace(); On 2015/11/17 03:12:51, Hector Carmona wrote: > On 2015/11/16 23:54:45, ncarter wrote: > > Have you tried just aliasing the value of web_contents->GetDelegate()? > > > > That should tell you (say, by looking at the vtable __vfptr under > VisualStudio) > > which subclass of content::WebContentsDelegate this WebContents was created > for. > > > > These are the subclasses: > > > https://code.google.com/p/chromium/codesearch#search/&q=%22public%20content::... > > > > This should work if the delegates always have their web contents created > in the same way. I got Browser as the delegate when I tried this out > with the print preview dialog on a regular window. Does that seem right? I don't know much about WebContentsDelegate, but sounds plausible to me. https://codereview.chromium.org/1432143002/diff/80001/components/constrained_... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/1432143002/diff/80001/components/constrained_... components/constrained_window/constrained_window_views.cc:152: base::debug::Alias(delegate); Have you verified that the vtable is captured in the crash dump with this technique? Can you get to the WebContentsDelegate pointer in any of the existing crash dumps? If so, you may already be able to look at the vtables.
On 2015/11/17 16:24:06, Mike Wittman wrote: > https://codereview.chromium.org/1432143002/diff/60001/components/constrained_... > File components/constrained_window/constrained_window_views.cc (right): > > https://codereview.chromium.org/1432143002/diff/60001/components/constrained_... > components/constrained_window/constrained_window_views.cc:152: > base::debug::StackTrace contents_stack_trace = web_contents->stack_trace(); > On 2015/11/17 03:12:51, Hector Carmona wrote: > > On 2015/11/16 23:54:45, ncarter wrote: > > > Have you tried just aliasing the value of web_contents->GetDelegate()? > > > > > > That should tell you (say, by looking at the vtable __vfptr under > > VisualStudio) > > > which subclass of content::WebContentsDelegate this WebContents was created > > for. > > > > > > These are the subclasses: > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=%22public%20content::... > > > > > > > This should work if the delegates always have their web contents created > > in the same way. I got Browser as the delegate when I tried this out > > with the print preview dialog on a regular window. Does that seem right? > > I don't know much about WebContentsDelegate, but sounds plausible to me. > > https://codereview.chromium.org/1432143002/diff/80001/components/constrained_... > File components/constrained_window/constrained_window_views.cc (right): > > https://codereview.chromium.org/1432143002/diff/80001/components/constrained_... > components/constrained_window/constrained_window_views.cc:152: > base::debug::Alias(delegate); > Have you verified that the vtable is captured in the crash dump with this > technique? > > Can you get to the WebContentsDelegate pointer in any of the existing crash > dumps? If so, you may already be able to look at the vtables. I tried looking for it, but it was optimized away in the minidumps.
nick@chromium.org changed reviewers: + nick@chromium.org - ncarter@chromium.org
lgtm https://codereview.chromium.org/1432143002/diff/80001/components/constrained_... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/1432143002/diff/80001/components/constrained_... components/constrained_window/constrained_window_views.cc:152: base::debug::Alias(delegate); On 2015/11/17 16:24:06, Mike Wittman wrote: > Have you verified that the vtable is captured in the crash dump with this > technique? > > Can you get to the WebContentsDelegate pointer in any of the existing crash > dumps? If so, you may already be able to look at the vtables. This should work; I wouldn't sweat it. For example, in the existing dumps you could see the vtable of |web_contents|, which is just another local variable captured by the dump.
lgtm
The CQ bit was checked by hcarmona@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1432143002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1432143002/80001
On 2015/11/17 03:12:51, Hector Carmona wrote: > I've removed other reviewers and set Mike as required. > > https://codereview.chromium.org/1432143002/diff/60001/components/constrained_... > File components/constrained_window/constrained_window_views.cc (right): > > https://codereview.chromium.org/1432143002/diff/60001/components/constrained_... > components/constrained_window/constrained_window_views.cc:152: > base::debug::StackTrace contents_stack_trace = web_contents->stack_trace(); > On 2015/11/16 23:54:45, ncarter wrote: > > Have you tried just aliasing the value of web_contents->GetDelegate()? > > > > That should tell you (say, by looking at the vtable __vfptr under > VisualStudio) > > which subclass of content::WebContentsDelegate this WebContents was created > for. > > > > These are the subclasses: > > > https://code.google.com/p/chromium/codesearch#search/&q=%22public%20content::... > > > > This should work if the delegates always have their web contents created > in the same way. I got Browser as the delegate when I tried this out > with the print preview dialog on a regular window. Does that seem right? It should at least narrow it down considerably (from dozens of possible call sites, to a handful). If I recall correctly, print preview is a strange case, where it adopts ownership of the webcontents that was originally created by a browser as a nested webui dialog. The only other case I can think of that kind of works like that (where WebContents ownership is transferred from one system to another) is Prerendering. The other big difference in looking at the dialog vs. looking at the stack trace, is that you won't see whether the webcontents was created via window.open() ( i.e. js-initiated creation, which gets handled via WebContentsDelegate::OpenURLFromTab) or not.
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e1158d6a42903de1664c12bc91ba056ecb716bb6 Cr-Commit-Position: refs/heads/master@{#360112}
Message was sent while issue was closed.
https://codereview.chromium.org/1432143002/diff/80001/components/constrained_... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/1432143002/diff/80001/components/constrained_... components/constrained_window/constrained_window_views.cc:152: base::debug::Alias(delegate); On 2015/11/17 17:42:16, ncarter wrote: > On 2015/11/17 16:24:06, Mike Wittman wrote: > > Have you verified that the vtable is captured in the crash dump with this > > technique? > > > > Can you get to the WebContentsDelegate pointer in any of the existing crash > > dumps? If so, you may already be able to look at the vtables. > > This should work; I wouldn't sweat it. For example, in the existing dumps you > could see the vtable of |web_contents|, which is just another local variable > captured by the dump. I wasn't able to see the variable in the new crash dumps in VS :-( delegate - Variable is optimized away and not available. go/vuehm
Message was sent while issue was closed.
Patch Set 4, didn't help us debug this. I know Patch Set 3 was a bit more complex because it added the stack trace to the web contents, but it might provide us with the debugging info that we need. Does it make sense to review and try to land Patch Set 3?
Message was sent while issue was closed.
On 2015/11/30 23:55:11, Hector Carmona wrote: > Patch Set 4, didn't help us debug this. I know Patch Set 3 was a bit > more complex because it added the stack trace to the web contents, but > it might provide us with the debugging info that we need. > > Does it make sense to review and try to land Patch Set 3? I'd say so, unless we have a better idea how to get the necessary information.
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:80001) has been created in https://codereview.chromium.org/1486863002/ by hcarmona@chromium.org. The reason for reverting is: Reverting Patch Set 4, since it was meant to be temporary anyways..
Message was sent while issue was closed.
Description was changed from ========== Track where WebContents are created in order to better understand issue. This is a temporary change to track down a beta blocking crash. This CL doesn't fix the crash, but will give us more information. The crash is happening when we attempt to show a modal dialog on some unknown WebContents that doesn't have a WebModalDialogManager. Not all WebContents are expecting to show modal dialogs and the creator of the WebModalDialog doesn't control what WebContents they will get at runtime. So, because of this loose coupling, we don't know which WebContents is triggering the crash. By adding this trace we expect to learn where the WebContents are created in order to make sure they have a WebModalDialogManager. BUG=538612 Committed: https://crrev.com/e1158d6a42903de1664c12bc91ba056ecb716bb6 Cr-Commit-Position: refs/heads/master@{#360112} ========== to ========== Track where WebContents are created in order to better understand issue. This is a temporary change to track down a beta blocking crash. This CL doesn't fix the crash, but will give us more information. The crash is happening when we attempt to show a modal dialog on some unknown WebContents that doesn't have a WebModalDialogManager. Not all WebContents are expecting to show modal dialogs and the creator of the WebModalDialog doesn't control what WebContents they will get at runtime. So, because of this loose coupling, we don't know which WebContents is triggering the crash. By adding this trace we expect to learn where the WebContents are created in order to make sure they have a WebModalDialogManager. BUG=538612 Committed: https://crrev.com/e1158d6a42903de1664c12bc91ba056ecb716bb6 Cr-Commit-Position: refs/heads/master@{#360112} ==========
The LGs on this are from Patch Set 4, PTAL at the latest version. (I can create a new CL if this one is too cluttered)
On 2015/12/01 00:58:28, Hector Carmona wrote: > The LGs on this are from Patch Set 4, PTAL at the latest version. (I can > create a new CL if this one is too cluttered) Please do create a new CL. Having two entirely different patches committed on the same CL will be very confusing. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
