Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1930323002:
Don't let Blink handle events if we're shuting down. (Closed)
DescriptionDon't let Blink handle events if we're shuting down.
We clear WebViewImpl::m_page when we're shutting down but the DOM and Frame
hierarchies are still around. We shouldn't handle any user input in this state.
BUG=607498
Committed: https://crrev.com/bb4a7319b4904705a6483f525149c8da9afcd248
Cr-Commit-Position: refs/heads/master@{#390719}
Patch Set 1 #
Total comments: 3
Messages
Total messages: 32 (11 generated)
Description was changed from ========== Don't let Blink handle events if we're shuting down. We clear WebViewImpl::m_page when we're shutting down but the DOM and Frame hierarchies are still around. We shouldn't handle any user input in this state. BUG=607001 ========== to ========== Don't let Blink handle events if we're shuting down. We clear WebViewImpl::m_page when we're shutting down but the DOM and Frame hierarchies are still around. We shouldn't handle any user input in this state. BUG=607001 ==========
bokan@chromium.org changed reviewers: + dtapuska@chromium.org
The impetus for this is that I was trying to read RootScroller off of FrameHost while handling a GestureScrollUpdate. Apparently, we can be handling a GSU while Page is null which means we're shutting down. Rather than just null check, I think we shouldn't be handling events in this state in the first place. I can't think of any issues this might cause, WDYT?
On 2016/04/29 02:02:04, bokan wrote: > The impetus for this is that I was trying to read RootScroller off of > FrameHost while handling a GestureScrollUpdate. Apparently, we can be > handling a GSU while Page is null which means we're shutting down. Rather > than just null check, I think we shouldn't be handling events in this > state in the first place. I can't think of any issues this might cause, > WDYT? lgtm. I can't think of a scenario that would be caused here by indicating it wasn't handled.
https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:317: if (!page()) Is it preferred to collapse this with the return below; like if (!page() || m_ignoreInputEvents)
https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:317: if (!page()) On 2016/04/29 14:31:27, dtapuska wrote: > Is it preferred to collapse this with the return below; like > > if (!page() || m_ignoreInputEvents) IMO, in this case, they're unrelated and it's more natural to comment the cases if they're separate.
On 2016/04/29 14:49:01, bokan wrote: > https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... > File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): > > https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... > third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:317: if (!page()) > On 2016/04/29 14:31:27, dtapuska wrote: > > Is it preferred to collapse this with the return below; like > > > > if (!page() || m_ignoreInputEvents) > > IMO, in this case, they're unrelated and it's more natural to comment the cases > if they're separate. sgtm
The CQ bit was checked by bokan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1930323002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1930323002/1
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:317: if (!page()) On 2016/04/29 at 14:49:00, bokan wrote: > On 2016/04/29 14:31:27, dtapuska wrote: > > Is it preferred to collapse this with the return below; like > > > > if (!page() || m_ignoreInputEvents) > > IMO, in this case, they're unrelated and it's more natural to comment the cases if they're separate. Dumb question: how come we don't hit this in the WebViewImpl case?
Description was changed from ========== Don't let Blink handle events if we're shuting down. We clear WebViewImpl::m_page when we're shutting down but the DOM and Frame hierarchies are still around. We shouldn't handle any user input in this state. BUG=607001 ========== to ========== Don't let Blink handle events if we're shuting down. We clear WebViewImpl::m_page when we're shutting down but the DOM and Frame hierarchies are still around. We shouldn't handle any user input in this state. BUG=607498 ==========
The CQ bit was unchecked by bokan@chromium.org
On 2016/04/29 18:36:30, dcheng wrote: > https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... > File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): > > https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... > third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:317: if (!page()) > On 2016/04/29 at 14:49:00, bokan wrote: > > On 2016/04/29 14:31:27, dtapuska wrote: > > > Is it preferred to collapse this with the return below; like > > > > > > if (!page() || m_ignoreInputEvents) > > > > IMO, in this case, they're unrelated and it's more natural to comment the > cases if they're separate. > > Dumb question: how come we don't hit this in the WebViewImpl case? Actually, good question and I hadn't noticed that. Looks like WebViewImpl::handleInputEvent already has this early-out but on m_client which is also cleared when shutting down: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Btw, this isn't a fix for the OOPIF bug, I had the wrong bug # linked and I've fixed that now.
The CQ bit was checked by bokan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1930323002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1930323002/1
On 2016/04/29 18:42:04, bokan wrote: > On 2016/04/29 18:36:30, dcheng wrote: > > > https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... > > File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): > > > > > https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... > > third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:317: if (!page()) > > On 2016/04/29 at 14:49:00, bokan wrote: > > > On 2016/04/29 14:31:27, dtapuska wrote: > > > > Is it preferred to collapse this with the return below; like > > > > > > > > if (!page() || m_ignoreInputEvents) > > > > > > IMO, in this case, they're unrelated and it's more natural to comment the > > cases if they're separate. > > > > Dumb question: how come we don't hit this in the WebViewImpl case? > > Actually, good question and I hadn't noticed that. Looks like > WebViewImpl::handleInputEvent already has this early-out but on m_client which Sorry, that early-out is in WebViewImpl::handleGestureEvent
On 2016/04/29 at 18:45:20, bokan wrote: > On 2016/04/29 18:42:04, bokan wrote: > > On 2016/04/29 18:36:30, dcheng wrote: > > > > > https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... > > > File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): > > > > > > > > https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... > > > third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:317: if (!page()) > > > On 2016/04/29 at 14:49:00, bokan wrote: > > > > On 2016/04/29 14:31:27, dtapuska wrote: > > > > > Is it preferred to collapse this with the return below; like > > > > > > > > > > if (!page() || m_ignoreInputEvents) > > > > > > > > IMO, in this case, they're unrelated and it's more natural to comment the > > > cases if they're separate. > > > > > > Dumb question: how come we don't hit this in the WebViewImpl case? > > > > Actually, good question and I hadn't noticed that. Looks like > > WebViewImpl::handleInputEvent already has this early-out but on m_client which > > Sorry, that early-out is in WebViewImpl::handleGestureEvent Is it possible to use m_client here for consistency?
On 2016/04/29 18:54:18, dcheng wrote: > On 2016/04/29 at 18:45:20, bokan wrote: > > On 2016/04/29 18:42:04, bokan wrote: > > > On 2016/04/29 18:36:30, dcheng wrote: > > > > > > > > https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... > > > > File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): > > > > > > > > > > > > https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... > > > > third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:317: if (!page()) > > > > On 2016/04/29 at 14:49:00, bokan wrote: > > > > > On 2016/04/29 14:31:27, dtapuska wrote: > > > > > > Is it preferred to collapse this with the return below; like > > > > > > > > > > > > if (!page() || m_ignoreInputEvents) > > > > > > > > > > IMO, in this case, they're unrelated and it's more natural to comment > the > > > > cases if they're separate. > > > > > > > > Dumb question: how come we don't hit this in the WebViewImpl case? > > > > > > Actually, good question and I hadn't noticed that. Looks like > > > WebViewImpl::handleInputEvent already has this early-out but on m_client > which > > > > Sorry, that early-out is in WebViewImpl::handleGestureEvent > > Is it possible to use m_client here for consistency? Sure, but is m_client better or should we use m_page in WebViewImpl?
On 2016/04/29 18:55:10, bokan wrote: > On 2016/04/29 18:54:18, dcheng wrote: > > On 2016/04/29 at 18:45:20, bokan wrote: > > > On 2016/04/29 18:42:04, bokan wrote: > > > > On 2016/04/29 18:36:30, dcheng wrote: > > > > > > > > > > > > https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... > > > > > File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... > > > > > third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:317: if (!page()) > > > > > On 2016/04/29 at 14:49:00, bokan wrote: > > > > > > On 2016/04/29 14:31:27, dtapuska wrote: > > > > > > > Is it preferred to collapse this with the return below; like > > > > > > > > > > > > > > if (!page() || m_ignoreInputEvents) > > > > > > > > > > > > IMO, in this case, they're unrelated and it's more natural to comment > > the > > > > > cases if they're separate. > > > > > > > > > > Dumb question: how come we don't hit this in the WebViewImpl case? > > > > > > > > Actually, good question and I hadn't noticed that. Looks like > > > > WebViewImpl::handleInputEvent already has this early-out but on m_client > > which > > > > > > Sorry, that early-out is in WebViewImpl::handleGestureEvent > > > > Is it possible to use m_client here for consistency? > > Sure, but is m_client better or should we use m_page in WebViewImpl? Actually, maybe we should just have an isShuttingDown() method on WVI to be more clear? WDYT?
The CQ bit was unchecked by bokan@chromium.org
On 2016/04/29 at 18:55:50, bokan wrote: > On 2016/04/29 18:55:10, bokan wrote: > > On 2016/04/29 18:54:18, dcheng wrote: > > > On 2016/04/29 at 18:45:20, bokan wrote: > > > > On 2016/04/29 18:42:04, bokan wrote: > > > > > On 2016/04/29 18:36:30, dcheng wrote: > > > > > > > > > > > > > > > > https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... > > > > > > File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... > > > > > > third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:317: if (!page()) > > > > > > On 2016/04/29 at 14:49:00, bokan wrote: > > > > > > > On 2016/04/29 14:31:27, dtapuska wrote: > > > > > > > > Is it preferred to collapse this with the return below; like > > > > > > > > > > > > > > > > if (!page() || m_ignoreInputEvents) > > > > > > > > > > > > > > IMO, in this case, they're unrelated and it's more natural to comment > > > the > > > > > > cases if they're separate. > > > > > > > > > > > > Dumb question: how come we don't hit this in the WebViewImpl case? > > > > > > > > > > Actually, good question and I hadn't noticed that. Looks like > > > > > WebViewImpl::handleInputEvent already has this early-out but on m_client > > > which > > > > > > > > Sorry, that early-out is in WebViewImpl::handleGestureEvent > > > > > > Is it possible to use m_client here for consistency? > > > > Sure, but is m_client better or should we use m_page in WebViewImpl? > > Actually, maybe we should just have an isShuttingDown() method on WVI to be more clear? WDYT? Different things get set to null at different points in shutdown, and I'm guessing the distinction matter sometimes. I suggested m_client because (I think) that should go to null before m_page does. But it's tricky: in this particular case, there's no obvious dependency on m_page or m_client and yet we have to null check one of them.
On 2016/04/29 19:05:02, dcheng wrote: > On 2016/04/29 at 18:55:50, bokan wrote: > > On 2016/04/29 18:55:10, bokan wrote: > > > On 2016/04/29 18:54:18, dcheng wrote: > > > > On 2016/04/29 at 18:45:20, bokan wrote: > > > > > On 2016/04/29 18:42:04, bokan wrote: > > > > > > On 2016/04/29 18:36:30, dcheng wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... > > > > > > > File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... > > > > > > > third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:317: if > (!page()) > > > > > > > On 2016/04/29 at 14:49:00, bokan wrote: > > > > > > > > On 2016/04/29 14:31:27, dtapuska wrote: > > > > > > > > > Is it preferred to collapse this with the return below; like > > > > > > > > > > > > > > > > > > if (!page() || m_ignoreInputEvents) > > > > > > > > > > > > > > > > IMO, in this case, they're unrelated and it's more natural to > comment > > > > the > > > > > > > cases if they're separate. > > > > > > > > > > > > > > Dumb question: how come we don't hit this in the WebViewImpl case? > > > > > > > > > > > > Actually, good question and I hadn't noticed that. Looks like > > > > > > WebViewImpl::handleInputEvent already has this early-out but on > m_client > > > > which > > > > > > > > > > Sorry, that early-out is in WebViewImpl::handleGestureEvent > > > > > > > > Is it possible to use m_client here for consistency? > > > > > > Sure, but is m_client better or should we use m_page in WebViewImpl? > > > > Actually, maybe we should just have an isShuttingDown() method on WVI to be > more clear? WDYT? > > Different things get set to null at different points in shutdown, and I'm > guessing the distinction matter sometimes. > > I suggested m_client because (I think) that should go to null before m_page > does. But it's tricky: in this particular case, there's no obvious dependency on > m_page or m_client and yet we have to null check one of them. Aren't they both cleared in WebViewImpl::close? Is WebViewImpl::close the true shutdown message? Should we just set a bit after we've called that?
On 2016/04/29 at 19:06:59, bokan wrote: > On 2016/04/29 19:05:02, dcheng wrote: > > On 2016/04/29 at 18:55:50, bokan wrote: > > > On 2016/04/29 18:55:10, bokan wrote: > > > > On 2016/04/29 18:54:18, dcheng wrote: > > > > > On 2016/04/29 at 18:45:20, bokan wrote: > > > > > > On 2016/04/29 18:42:04, bokan wrote: > > > > > > > On 2016/04/29 18:36:30, dcheng wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... > > > > > > > > File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... > > > > > > > > third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:317: if > > (!page()) > > > > > > > > On 2016/04/29 at 14:49:00, bokan wrote: > > > > > > > > > On 2016/04/29 14:31:27, dtapuska wrote: > > > > > > > > > > Is it preferred to collapse this with the return below; like > > > > > > > > > > > > > > > > > > > > if (!page() || m_ignoreInputEvents) > > > > > > > > > > > > > > > > > > IMO, in this case, they're unrelated and it's more natural to > > comment > > > > > the > > > > > > > > cases if they're separate. > > > > > > > > > > > > > > > > Dumb question: how come we don't hit this in the WebViewImpl case? > > > > > > > > > > > > > > Actually, good question and I hadn't noticed that. Looks like > > > > > > > WebViewImpl::handleInputEvent already has this early-out but on > > m_client > > > > > which > > > > > > > > > > > > Sorry, that early-out is in WebViewImpl::handleGestureEvent > > > > > > > > > > Is it possible to use m_client here for consistency? > > > > > > > > Sure, but is m_client better or should we use m_page in WebViewImpl? > > > > > > Actually, maybe we should just have an isShuttingDown() method on WVI to be > > more clear? WDYT? > > > > Different things get set to null at different points in shutdown, and I'm > > guessing the distinction matter sometimes. > > > > I suggested m_client because (I think) that should go to null before m_page > > does. But it's tricky: in this particular case, there's no obvious dependency on > > m_page or m_client and yet we have to null check one of them. > > Aren't they both cleared in WebViewImpl::close? Is WebViewImpl::close the true shutdown message? Should we just set a bit after we've called that? Yes, but I think it's actually possible (and permitted) to pass null as a client... Though maybe that's an argument for using m_page... I would say let's worry about this in a future patch, this entire area is a mess.
The CQ bit was checked by bokan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1930323002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1930323002/1
On 2016/04/29 19:13:16, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1930323002/1 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1930323002/1
On 2016/04/29 19:12:30, dcheng wrote: > On 2016/04/29 at 19:06:59, bokan wrote: > > On 2016/04/29 19:05:02, dcheng wrote: > > > On 2016/04/29 at 18:55:50, bokan wrote: > > > > On 2016/04/29 18:55:10, bokan wrote: > > > > > On 2016/04/29 18:54:18, dcheng wrote: > > > > > > On 2016/04/29 at 18:45:20, bokan wrote: > > > > > > > On 2016/04/29 18:42:04, bokan wrote: > > > > > > > > On 2016/04/29 18:36:30, dcheng wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... > > > > > > > > > File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1930323002/diff/1/third_party/WebKit/Source/w... > > > > > > > > > third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:317: if > > > (!page()) > > > > > > > > > On 2016/04/29 at 14:49:00, bokan wrote: > > > > > > > > > > On 2016/04/29 14:31:27, dtapuska wrote: > > > > > > > > > > > Is it preferred to collapse this with the return below; like > > > > > > > > > > > > > > > > > > > > > > if (!page() || m_ignoreInputEvents) > > > > > > > > > > > > > > > > > > > > IMO, in this case, they're unrelated and it's more natural to > > > comment > > > > > > the > > > > > > > > > cases if they're separate. > > > > > > > > > > > > > > > > > > Dumb question: how come we don't hit this in the WebViewImpl > case? > > > > > > > > > > > > > > > > Actually, good question and I hadn't noticed that. Looks like > > > > > > > > WebViewImpl::handleInputEvent already has this early-out but on > > > m_client > > > > > > which > > > > > > > > > > > > > > Sorry, that early-out is in WebViewImpl::handleGestureEvent > > > > > > > > > > > > Is it possible to use m_client here for consistency? > > > > > > > > > > Sure, but is m_client better or should we use m_page in WebViewImpl? > > > > > > > > Actually, maybe we should just have an isShuttingDown() method on WVI to > be > > > more clear? WDYT? > > > > > > Different things get set to null at different points in shutdown, and I'm > > > guessing the distinction matter sometimes. > > > > > > I suggested m_client because (I think) that should go to null before m_page > > > does. But it's tricky: in this particular case, there's no obvious > dependency on > > > m_page or m_client and yet we have to null check one of them. > > > > Aren't they both cleared in WebViewImpl::close? Is WebViewImpl::close the true > shutdown message? Should we just set a bit after we've called that? > > Yes, but I think it's actually possible (and permitted) to pass null as a > client... > Though maybe that's an argument for using m_page... > I would say let's worry about this in a future patch, this entire area is a > mess. Ok, filed crbug.com/608005 to track that.
Message was sent while issue was closed.
Description was changed from ========== Don't let Blink handle events if we're shuting down. We clear WebViewImpl::m_page when we're shutting down but the DOM and Frame hierarchies are still around. We shouldn't handle any user input in this state. BUG=607498 ========== to ========== Don't let Blink handle events if we're shuting down. We clear WebViewImpl::m_page when we're shutting down but the DOM and Frame hierarchies are still around. We shouldn't handle any user input in this state. BUG=607498 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/bb4a7319b4904705a6483f525149c8da9afcd248 Cr-Commit-Position: refs/heads/master@{#390719}
Message was sent while issue was closed.
Description was changed from ========== Don't let Blink handle events if we're shuting down. We clear WebViewImpl::m_page when we're shutting down but the DOM and Frame hierarchies are still around. We shouldn't handle any user input in this state. BUG=607498 ========== to ========== Don't let Blink handle events if we're shuting down. We clear WebViewImpl::m_page when we're shutting down but the DOM and Frame hierarchies are still around. We shouldn't handle any user input in this state. BUG=607498 Committed: https://crrev.com/bb4a7319b4904705a6483f525149c8da9afcd248 Cr-Commit-Position: refs/heads/master@{#390719} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
