Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1622633003:
[DevTools] Check that mainFrameImpl() has been initalized before accessing it. (Closed)
|
Created:
4 years, 11 months ago by dgozman Modified:
4 years, 10 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, sergeyv+blink_chromium.org, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Check that mainFrameImpl() has been initalized before accessing it.
During reattach, InspectorEmulationAgent tries to access
mainFrameImpl() too early, which happens to break during
remote -> local main frame transition.
Similar thing happens when we cleanup during local -> remote transition.
Somehow, we also enable emulation for remote frames sometimes (or just
not disbale it, so it keeps going). Added null-check for now and TODO.
BUG=579065, 579474, 578968, 543896
Committed: https://crrev.com/228893becc4c3c8936f80a1f5a9885ef7095e216
Cr-Commit-Position: refs/heads/master@{#372200}
Patch Set 1 #
Total comments: 3
Patch Set 2 : more checks, better comments #Messages
Total messages: 16 (6 generated)
dgozman@chromium.org changed reviewers: + nasko@chromium.org
Nasko, could you please take a look? I think we are mostly doing the right thing, which is routing emulation through the view/page (as it's a main-frame-only concept). What is wrong here is that we try to setup things in reattach call which happens very early (so we can inspect from the very start), but some things like mainFrameImpl() are not ready yet. We can instead go through browser and make adjustments after everything was set up. Or wait with reattach in renderer until just after all frame/page/view setup is correct. This requires some work though, so meanwhile easy fix to merge to M49.
dgozman@chromium.org changed reviewers: + dcheng@chromium.org
Friendly ping. Adding Daniel, in case Nasko is unavailable.
I'm not sure my knowledge of DevTools is good enough to judge the validity of the fix. It definitely will fix the problem when the main frame is remote. I've made a note on the comment, as I don't think it is entirely correct. https://codereview.chromium.org/1622633003/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/DevToolsEmulator.cpp (right): https://codereview.chromium.org/1622633003/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/DevToolsEmulator.cpp:324: // local main frame, and remove InspectorEmulationAgent entirely. WebViewImpl is not guaranteed to have local main frame in a process that renders subframes.
Description was changed from ========== [DevTools] Check that mainFrameImpl() has been initalized before accessing it. During reattach, InspectorEmulationAgent tries to access mainFrameImpl() too early, which happens to break during remote -> local main frame transition. BUG=579065 ========== to ========== [DevTools] Check that mainFrameImpl() has been initalized before accessing it. During reattach, InspectorEmulationAgent tries to access mainFrameImpl() too early, which happens to break during remote -> local main frame transition. Similar thing happens when we cleanup during local -> remote transition. Somehow, we also enable emulation for remote frames sometimes (or just not disbale it, so it keeps going). Added null-check for now and TODO. BUG=579065,579474,578968,543896 ==========
dgozman@chromium.org changed reviewers: + pfeldman@chromium.org
+pfeldman for devtools. https://codereview.chromium.org/1622633003/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/DevToolsEmulator.cpp (right): https://codereview.chromium.org/1622633003/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/DevToolsEmulator.cpp:324: // local main frame, and remove InspectorEmulationAgent entirely. On 2016/01/26 19:43:07, nasko wrote: > WebViewImpl is not guaranteed to have local main frame in a process that renders > subframes. Ah, right. But if we send the page-level IPC from browser it should go to the local main frame, shouldn't it? I rephrased the comment.
https://codereview.chromium.org/1622633003/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/DevToolsEmulator.cpp (right): https://codereview.chromium.org/1622633003/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/DevToolsEmulator.cpp:324: // local main frame, and remove InspectorEmulationAgent entirely. On 2016/01/27 17:54:44, dgozman wrote: > On 2016/01/26 19:43:07, nasko wrote: > > WebViewImpl is not guaranteed to have local main frame in a process that > renders > > subframes. > > Ah, right. But if we send the page-level IPC from browser it should go to the > local main frame, shouldn't it? I rephrased the comment. Page-level IPCs in general should go to all processes, so they can all have the right state. Otherwise your main frame will be in emulation mode, but the subframes will not be, right?
On 2016/01/27 17:55:57, nasko wrote: > https://codereview.chromium.org/1622633003/diff/1/third_party/WebKit/Source/w... > File third_party/WebKit/Source/web/DevToolsEmulator.cpp (right): > > https://codereview.chromium.org/1622633003/diff/1/third_party/WebKit/Source/w... > third_party/WebKit/Source/web/DevToolsEmulator.cpp:324: // local main frame, and > remove InspectorEmulationAgent entirely. > On 2016/01/27 17:54:44, dgozman wrote: > > On 2016/01/26 19:43:07, nasko wrote: > > > WebViewImpl is not guaranteed to have local main frame in a process that > > renders > > > subframes. > > > > Ah, right. But if we send the page-level IPC from browser it should go to the > > local main frame, shouldn't it? I rephrased the comment. > > Page-level IPCs in general should go to all processes, so they can all have the > right state. Otherwise your main frame will be in emulation mode, but the > subframes will not be, right? That's a hard question. Generally, emulation is top-frame only, but some things should be tweaked in subframes as well. I will walk over what we have with this in mind. Perhaps, it would be better to orchestrate the whole thing from the single entry point in browser and send different IPCs to the main frame and to subframes. In this case all the null checks I've added make perfect sense - we just do something extra for local main frame.
lgtm
The CQ bit was checked by dgozman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1622633003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1622633003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Check that mainFrameImpl() has been initalized before accessing it. During reattach, InspectorEmulationAgent tries to access mainFrameImpl() too early, which happens to break during remote -> local main frame transition. Similar thing happens when we cleanup during local -> remote transition. Somehow, we also enable emulation for remote frames sometimes (or just not disbale it, so it keeps going). Added null-check for now and TODO. BUG=579065,579474,578968,543896 ========== to ========== [DevTools] Check that mainFrameImpl() has been initalized before accessing it. During reattach, InspectorEmulationAgent tries to access mainFrameImpl() too early, which happens to break during remote -> local main frame transition. Similar thing happens when we cleanup during local -> remote transition. Somehow, we also enable emulation for remote frames sometimes (or just not disbale it, so it keeps going). Added null-check for now and TODO. BUG=579065,579474,578968,543896 Committed: https://crrev.com/228893becc4c3c8936f80a1f5a9885ef7095e216 Cr-Commit-Position: refs/heads/master@{#372200} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/228893becc4c3c8936f80a1f5a9885ef7095e216 Cr-Commit-Position: refs/heads/master@{#372200} |
