Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1855423002:
Make USB a ContextLifecycleObserver. (Closed)
|
Created:
4 years, 8 months ago by Reilly Grant (use Gerrit) Modified:
4 years, 8 months ago CC:
blink-reviews, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake USB a ContextLifecycleObserver.
This patch makes USB a ContextLifecycleObserver. When the context is
destroyed it stops listening for device changes. This is the same signal
USBDevice listens for to close device connections on navigation.
BUG=598855
Committed: https://crrev.com/54ad52b6a815e0aaca6391f7baac2e1064d4acec
Cr-Commit-Position: refs/heads/master@{#385365}
Patch Set 1 #Patch Set 2 : Switch back to ContextLifetimeObservers. #
Total comments: 4
Messages
Total messages: 27 (6 generated)
reillyg@chromium.org changed reviewers: + jyasskin@chromium.org
Please take a look.
This change makes some sense, but the CL description doesn't make sense. > There objects were previously ContextLifecycleObservers but this was not ContextLifecycleObservers => LocalFrameLifecycleObserver > not good enough because ContextLifecycleObserver::willDetachFrameHost() > is not called on navigation. LocalFrameLifecycleObserver::willDetachFrameHost() must be called when a frame navigates away. > To detect when the page has been stopped If you want to observe the page stop, you need to use PageLifecycleObserver. > USB and USBDevice should be ActiveDOMObjects and perform their cleanup > work in ActiveDOMObject::stop(). What the CL is doing is to observe the ExecutionContext stop. Also, would you add a test case for this change?
On 2016/04/05 at 01:17:41, haraken wrote: > > not good enough because ContextLifecycleObserver::willDetachFrameHost() > > is not called on navigation. > > LocalFrameLifecycleObserver::willDetachFrameHost() must be called when a frame navigates away. I can demonstrate that this is not true. I am currently experiencing a crash because we can end up with two USB objects that are both still observing their client which would not be the case if USB::willDetachFrameHost() where being called. > > To detect when the page has been stopped > > If you want to observe the page stop, you need to use PageLifecycleObserver. > > > USB and USBDevice should be ActiveDOMObjects and perform their cleanup > > work in ActiveDOMObject::stop(). > > What the CL is doing is to observe the ExecutionContext stop. Can you point me to documentation that explains the difference? > Also, would you add a test case for this change? I will try.
On 2016/04/05 at 01:28:25, Reilly Grant wrote: > On 2016/04/05 at 01:17:41, haraken wrote: > > > not good enough because ContextLifecycleObserver::willDetachFrameHost() > > > is not called on navigation. > > > > LocalFrameLifecycleObserver::willDetachFrameHost() must be called when a frame navigates away. > > I can demonstrate that this is not true. I am currently experiencing a crash because we can end up with two USB objects that are both still observing their client which would not be the case if USB::willDetachFrameHost() where being called. To be more specific, I either get a crash (3febfa8c00000000) or, when I add logs, I see both USBs continuing to dispatch events. I'm not sure why I'm not getting the same crash when I do that.
Description was changed from ========== Make USB and USBDevice ActiveDOMObjects. There objects were previously ContextLifecycleObservers but this was not not good enough because ContextLifecycleObserver::willDetachFrameHost() is not called on navigation. To detect when the page has been stopped USB and USBDevice should be ActiveDOMObjects and perform their cleanup work in ActiveDOMObject::stop(). BUG=None ========== to ========== Make USB a ContextLifecycleObserver. This patch makes USB a ContextLifecycleObserver. When the context is destroyed it stops listening for device changes. This is the same signal USBDevice listens for to close device connections on navigation. BUG=598855 ==========
I've updated this patch to simply make USB a ContextLifecycleObserver like USBDevice and verified that this is as good as making it an ActiveDOMObject and implementing stop(). Unfortunately I can't yet add a test for this because the mock Mojo service implementation has to run in the same frame as the test. I'll talk to the Mojo team about allowing a frame to override services for iframes within it.
jyasskin@chromium.org changed reviewers: + haraken@chromium.org
Kentaro, could you review this? Without any documentation or tests for LifecycleObserver, I don't know enough to even guess whether it's correct.
>> LocalFrameLifecycleObserver::willDetachFrameHost() must be called when a frame navigates away. >> > I can demonstrate that this is not true. I am currently experiencing a crash > because we can end up with two USB objects that are both still observing their > client which would not be the case if USB::willDetachFrameHost() where being > called. Is the frame really detached? If the frame doesn't get detached, USB::willDetachFrameHost() won't be called.
https://codereview.chromium.org/1855423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webusb/USB.cpp (right): https://codereview.chromium.org/1855423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webusb/USB.cpp:79: , m_client(USBController::from(frame).client()) It looks strange that USB is observing ExecutionContext while USBController is associated with Frame. Would you explain the relationship between Page (which doesn't change over navigation), Frame (which changes over navigation), ExecutionContext (which is actually a document in the frame) and USB things?
On 2016/04/06 at 00:15:07, haraken wrote: > >> LocalFrameLifecycleObserver::willDetachFrameHost() must be called when a frame > navigates away. > >> > > I can demonstrate that this is not true. I am currently experiencing a crash > > because we can end up with two USB objects that are both still observing their > > client which would not be the case if USB::willDetachFrameHost() where being > > called. > > Is the frame really detached? If the frame doesn't get detached, USB::willDetachFrameHost() won't be called. What causes a frame to be detached? It appears not be detached when window.location.href is set to a different URL which is the case causing the behavior I am observing.
https://codereview.chromium.org/1855423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webusb/USB.cpp (right): https://codereview.chromium.org/1855423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webusb/USB.cpp:79: , m_client(USBController::from(frame).client()) On 2016/04/06 at 00:15:12, haraken wrote: > It looks strange that USB is observing ExecutionContext while USBController is associated with Frame. Would you explain the relationship between Page (which doesn't change over navigation), Frame (which changes over navigation), ExecutionContext (which is actually a document in the frame) and USB things? The Frame is what provides a connection to Mojo services in Chrome. Currently that's mediated through the WebUSBClient provided by the USBController but very soon this constructor will simply request an instance of the device::usb::DeviceManager Mojo service from frame.serviceRegistry(). I believe that the USB objects in Blink, however, should be owned by a particular ExecutionContext. I don't want to depend on GC to clean them up because if the page is refreshed I want old device connections to be cleaned up immediately so that the page can reconnect as soon as it loads.
haraken@chromium.org changed reviewers: + dcheng@chromium.org
On 2016/04/06 00:54:27, Reilly Grant wrote: > On 2016/04/06 at 00:15:07, haraken wrote: > > >> LocalFrameLifecycleObserver::willDetachFrameHost() must be called when a > frame > > navigates away. > > >> > > > I can demonstrate that this is not true. I am currently experiencing a crash > > > because we can end up with two USB objects that are both still observing > their > > > client which would not be the case if USB::willDetachFrameHost() where being > > > called. > > > > Is the frame really detached? If the frame doesn't get detached, > USB::willDetachFrameHost() won't be called. > > What causes a frame to be detached? It appears not be detached when > window.location.href is set to a different URL which is the case causing the > behavior I am observing. +dcheng (BTW, PS2 looks good. I just want to confirm we're not misunderstanding things.)
On 2016/04/06 at 01:04:51, haraken wrote: > On 2016/04/06 00:54:27, Reilly Grant wrote: > > On 2016/04/06 at 00:15:07, haraken wrote: > > > >> LocalFrameLifecycleObserver::willDetachFrameHost() must be called when a > > frame > > > navigates away. > > > >> > > > > I can demonstrate that this is not true. I am currently experiencing a crash > > > > because we can end up with two USB objects that are both still observing > > their > > > > client which would not be the case if USB::willDetachFrameHost() where being > > > > called. > > > > > > Is the frame really detached? If the frame doesn't get detached, > > USB::willDetachFrameHost() won't be called. > > > > What causes a frame to be detached? It appears not be detached when > > window.location.href is set to a different URL which is the case causing the > > behavior I am observing. > > +dcheng > > (BTW, PS2 looks good. I just want to confirm we're not misunderstanding things.) Frames aren't (usually*) detached on navigation: we reuse Frame but create a new Window/Document pair for the navigation: that's why willDetachFrameHost() wasn't called. I guess we should document this somewhere, as it is rather confusing.
On 2016/04/06 at 01:14:24, dcheng wrote: > On 2016/04/06 at 01:04:51, haraken wrote: > > On 2016/04/06 00:54:27, Reilly Grant wrote: > > > On 2016/04/06 at 00:15:07, haraken wrote: > > > > >> LocalFrameLifecycleObserver::willDetachFrameHost() must be called when a > > > frame > > > > navigates away. > > > > >> > > > > > I can demonstrate that this is not true. I am currently experiencing a crash > > > > > because we can end up with two USB objects that are both still observing > > > their > > > > > client which would not be the case if USB::willDetachFrameHost() where being > > > > > called. > > > > > > > > Is the frame really detached? If the frame doesn't get detached, > > > USB::willDetachFrameHost() won't be called. > > > > > > What causes a frame to be detached? It appears not be detached when > > > window.location.href is set to a different URL which is the case causing the > > > behavior I am observing. > > > > +dcheng > > > > (BTW, PS2 looks good. I just want to confirm we're not misunderstanding things.) > > Frames aren't (usually*) detached on navigation: we reuse Frame but create a new Window/Document pair for the navigation: that's why willDetachFrameHost() wasn't called. I guess we should document this somewhere, as it is rather confusing. The * is for out-of-process iframes =)
LGTM https://codereview.chromium.org/1855423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webusb/USB.h (right): https://codereview.chromium.org/1855423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webusb/USB.h:30: static USB* create(LocalFrame& frame) Can you make USB::create take Document instead of LocalFrame? Now USB is not related to LocalFrame.
I'm afraid that many callers are expecting willDetachFrameHost is called on navigation... https://code.google.com/p/chromium/codesearch#search/&q=willdetachframehost&s... (Either way, we need to clean up the family of LifecycleObservers at some point. People are using them without fully understanding when the lifecycle callbacks are called.)
https://codereview.chromium.org/1855423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webusb/USB.h (right): https://codereview.chromium.org/1855423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webusb/USB.h:30: static USB* create(LocalFrame& frame) On 2016/04/06 at 01:15:42, haraken wrote: > Can you make USB::create take Document instead of LocalFrame? Now USB is not related to LocalFrame. I cannot because I need the USBController (or in the future, the ServiceRegistry) which is attached to the frame.
The CQ bit was checked by reillyg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855423002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855423002/20001
On 2016/04/06 at 01:18:38, haraken wrote: > I'm afraid that many callers are expecting willDetachFrameHost is called on navigation... > > https://code.google.com/p/chromium/codesearch#search/&q=willdetachframehost&s... > > (Either way, we need to clean up the family of LifecycleObservers at some point. People are using them without fully understanding when the lifecycle callbacks are called.) Yeah, it looks like some of these are OK but some might not be. Unfortunately, we are setting people up for failure: the way supplements are created is usually at the frame level in WebLocalFrameImpl::setCoreFrame... but objects exposed to script should usually be associated with an ExecutionContext since they shouldn't be valid after navigation. =/
On 2016/04/06 at 01:21:56, reillyg wrote: > https://codereview.chromium.org/1855423002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/webusb/USB.h (right): > > https://codereview.chromium.org/1855423002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webusb/USB.h:30: static USB* create(LocalFrame& frame) > On 2016/04/06 at 01:15:42, haraken wrote: > > Can you make USB::create take Document instead of LocalFrame? Now USB is not related to LocalFrame. > > I cannot because I need the USBController (or in the future, the ServiceRegistry) which is attached to the frame. Actually, it's doable but awkward. FrameLoaderClient::transitionToCommittedForNewPage() could be used to create Document supplements. This is probably something the architecture team should figure out though. I think we need to figure out some guidelines around when something should be frame supplements vs document supplements: objects should not be frame supplements just to make it easier to get to something on a frame.
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Make USB a ContextLifecycleObserver. This patch makes USB a ContextLifecycleObserver. When the context is destroyed it stops listening for device changes. This is the same signal USBDevice listens for to close device connections on navigation. BUG=598855 ========== to ========== Make USB a ContextLifecycleObserver. This patch makes USB a ContextLifecycleObserver. When the context is destroyed it stops listening for device changes. This is the same signal USBDevice listens for to close device connections on navigation. BUG=598855 Committed: https://crrev.com/54ad52b6a815e0aaca6391f7baac2e1064d4acec Cr-Commit-Position: refs/heads/master@{#385365} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/54ad52b6a815e0aaca6391f7baac2e1064d4acec Cr-Commit-Position: refs/heads/master@{#385365} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
