Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1848603002:
Remove single observer assumption in WebUSBClient. (Closed)
|
Created:
4 years, 8 months ago by Reilly Grant (use Gerrit) Modified:
4 years, 8 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove single observer assumption in WebUSBClient.
This patch removes the assumption in WebUSBClient that there will be a
single instance of blink::USB active at once. Delayed garbage collection
can mean that an old instance exists for some time after a new instance
has added itself as an observer. With only a single observer slot the
old instance could accidentally unregister the new instance when it was
destroyed.
BUG=598855
Committed: https://crrev.com/e0584330d7c9ddf1887547d58cc2a7e0cb92fe1b
Cr-Commit-Position: refs/heads/master@{#384763}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Add a pre-finalizer to USB. #
Total comments: 2
Patch Set 3 : Register pre-finalizer. #
Total comments: 1
Patch Set 4 : DCHECK that m_client has been cleared before destruction. #
Total comments: 2
Patch Set 5 : Use EAGERLY_FINALIZE() instead. #
Total comments: 1
Messages
Total messages: 29 (9 generated)
reillyg@chromium.org changed reviewers: + haraken@chromium.org
haraken@, please take a look. I believed before investigating this that the WebUSBClient would not be reused like this but that is apparently not the case.
haraken@chromium.org changed reviewers: + oilpan-reviews@chromium.org
https://codereview.chromium.org/1848603002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webusb/USB.cpp (right): https://codereview.chromium.org/1848603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webusb/USB.cpp:88: m_client->removeObserver(this); Is it possible that USB's destructor is called without USB::willDetachFrameHost() getting called? If that is possible, I think you need to make sure that m_client->removeObserver(this) is called before GC's sweeping phase starts. Otherwise, the following scenario can happen: 1) GC is triggered. USB becomes unreachable. 2) Lazy sweeping starts. It destructs some unreachable object. Assume that USB isn't yet destructed. 3) m_client accesses the already destructed object. To avoid that from happening, you need to make sure that m_client->removeObserver(this) is called before the sweeping phase starts. Specifically, you need to add USING_PRE_FINALIZER() and call m_client->removeObserver(this) in the pre-finalizer.
https://codereview.chromium.org/1848603002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webusb/USB.cpp (right): https://codereview.chromium.org/1848603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webusb/USB.cpp:88: m_client->removeObserver(this); On 2016/03/30 at 23:35:23, haraken wrote: > Is it possible that USB's destructor is called without USB::willDetachFrameHost() getting called? USB::willDetachFrameHost() doesn't appear to be being called at all. Do you know why? It violates assumptions I had about LocalFrame lifetimes. > If that is possible, I think you need to make sure that m_client->removeObserver(this) is called before GC's sweeping phase starts. Otherwise, the following scenario can happen: > > 1) GC is triggered. USB becomes unreachable. > 2) Lazy sweeping starts. It destructs some unreachable object. Assume that USB isn't yet destructed. > 3) m_client accesses the already destructed object. > > To avoid that from happening, you need to make sure that m_client->removeObserver(this) is called before the sweeping phase starts. Specifically, you need to add USING_PRE_FINALIZER() and call m_client->removeObserver(this) in the pre-finalizer. Okay.
https://codereview.chromium.org/1848603002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webusb/USB.cpp (right): https://codereview.chromium.org/1848603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webusb/USB.cpp:88: m_client->removeObserver(this); On 2016/03/30 23:38:10, Reilly Grant wrote: > On 2016/03/30 at 23:35:23, haraken wrote: > > Is it possible that USB's destructor is called without > USB::willDetachFrameHost() getting called? > > USB::willDetachFrameHost() doesn't appear to be being called at all. Do you know > why? It violates assumptions I had about LocalFrame lifetimes. It must be called. Is the USB object associated with a valid frame? > > > If that is possible, I think you need to make sure that > m_client->removeObserver(this) is called before GC's sweeping phase starts. > Otherwise, the following scenario can happen: > > > > 1) GC is triggered. USB becomes unreachable. > > 2) Lazy sweeping starts. It destructs some unreachable object. Assume that USB > isn't yet destructed. > > 3) m_client accesses the already destructed object. > > > > To avoid that from happening, you need to make sure that > m_client->removeObserver(this) is called before the sweeping phase starts. > Specifically, you need to add USING_PRE_FINALIZER() and call > m_client->removeObserver(this) in the pre-finalizer. > > Okay.
https://codereview.chromium.org/1848603002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webusb/USB.cpp (right): https://codereview.chromium.org/1848603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webusb/USB.cpp:88: m_client->removeObserver(this); On 2016/03/30 at 23:44:05, haraken wrote: > On 2016/03/30 23:38:10, Reilly Grant wrote: > > On 2016/03/30 at 23:35:23, haraken wrote: > > > Is it possible that USB's destructor is called without > > USB::willDetachFrameHost() getting called? > > > > USB::willDetachFrameHost() doesn't appear to be being called at all. Do you know > > why? It violates assumptions I had about LocalFrame lifetimes. > > It must be called. Is the USB object associated with a valid frame? I have tested again and confirmed that USB::willDetachFrameHost() is not called before USB::~USB(). How would I know if the frame was valid? The LocalFrameLifecycleObserver is initialized using the LocalFrame passed to USB::USB() which is Navigator::frame() and guaranteed to be non-null. > > > If that is possible, I think you need to make sure that > > m_client->removeObserver(this) is called before GC's sweeping phase starts. > > Otherwise, the following scenario can happen: > > > > > > 1) GC is triggered. USB becomes unreachable. > > > 2) Lazy sweeping starts. It destructs some unreachable object. Assume that USB > > isn't yet destructed. > > > 3) m_client accesses the already destructed object. > > > > > > To avoid that from happening, you need to make sure that > > m_client->removeObserver(this) is called before the sweeping phase starts. > > Specifically, you need to add USING_PRE_FINALIZER() and call > > m_client->removeObserver(this) in the pre-finalizer. Looking at this again I'm not sure I understand why a pre-finalizer is necessary. Are you saying that USB::onDeviceAdded or USB::onDeviceRemoved may touch an unreachable object because after USB is declared unreachable an object it refers to may be destroyed but it itself may not be?
On 2016/03/31 23:12:57, Reilly Grant wrote: > https://codereview.chromium.org/1848603002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/webusb/USB.cpp (right): > > https://codereview.chromium.org/1848603002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/webusb/USB.cpp:88: > m_client->removeObserver(this); > On 2016/03/30 at 23:44:05, haraken wrote: > > On 2016/03/30 23:38:10, Reilly Grant wrote: > > > On 2016/03/30 at 23:35:23, haraken wrote: > > > > Is it possible that USB's destructor is called without > > > USB::willDetachFrameHost() getting called? > > > > > > USB::willDetachFrameHost() doesn't appear to be being called at all. Do you > know > > > why? It violates assumptions I had about LocalFrame lifetimes. > > > > It must be called. Is the USB object associated with a valid frame? > > I have tested again and confirmed that USB::willDetachFrameHost() is not called > before USB::~USB(). How would I know if the frame was valid? The > LocalFrameLifecycleObserver is initialized using the LocalFrame passed to > USB::USB() which is Navigator::frame() and guaranteed to be non-null. > > > > > If that is possible, I think you need to make sure that > > > m_client->removeObserver(this) is called before GC's sweeping phase starts. > > > Otherwise, the following scenario can happen: > > > > > > > > 1) GC is triggered. USB becomes unreachable. > > > > 2) Lazy sweeping starts. It destructs some unreachable object. Assume that > USB > > > isn't yet destructed. > > > > 3) m_client accesses the already destructed object. > > > > > > > > To avoid that from happening, you need to make sure that > > > m_client->removeObserver(this) is called before the sweeping phase starts. > > > Specifically, you need to add USING_PRE_FINALIZER() and call > > > m_client->removeObserver(this) in the pre-finalizer. > > Looking at this again I'm not sure I understand why a pre-finalizer is > necessary. Are you saying that USB::onDeviceAdded or USB::onDeviceRemoved may > touch an unreachable object because after USB is declared unreachable an object > it refers to may be destroyed but it itself may not be? Correct. In general, you need to clear all raw pointers (in content/ side) to on-heap objects (USB) before the lazy sweeping phase starts. Otherwise, the raw pointers can be stale and case use-after-free.
https://codereview.chromium.org/1848603002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webusb/USB.cpp (right): https://codereview.chromium.org/1848603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webusb/USB.cpp:88: m_client->removeObserver(this); On 2016/03/31 23:12:57, Reilly Grant wrote: > On 2016/03/30 at 23:44:05, haraken wrote: > > On 2016/03/30 23:38:10, Reilly Grant wrote: > > > On 2016/03/30 at 23:35:23, haraken wrote: > > > > Is it possible that USB's destructor is called without > > > USB::willDetachFrameHost() getting called? > > > > > > USB::willDetachFrameHost() doesn't appear to be being called at all. Do you > know > > > why? It violates assumptions I had about LocalFrame lifetimes. > > > > It must be called. Is the USB object associated with a valid frame? > > I have tested again and confirmed that USB::willDetachFrameHost() is not called > before USB::~USB(). How would I know if the frame was valid? The > LocalFrameLifecycleObserver is initialized using the LocalFrame passed to > USB::USB() which is Navigator::frame() and guaranteed to be non-null. > > > > > If that is possible, I think you need to make sure that > > > m_client->removeObserver(this) is called before GC's sweeping phase starts. > > > Otherwise, the following scenario can happen: > > > > > > > > 1) GC is triggered. USB becomes unreachable. > > > > 2) Lazy sweeping starts. It destructs some unreachable object. Assume that > USB > > > isn't yet destructed. > > > > 3) m_client accesses the already destructed object. > > > > > > > > To avoid that from happening, you need to make sure that > > > m_client->removeObserver(this) is called before the sweeping phase starts. > > > Specifically, you need to add USING_PRE_FINALIZER() and call > > > m_client->removeObserver(this) in the pre-finalizer. > > Looking at this again I'm not sure I understand why a pre-finalizer is > necessary. Are you saying that USB::onDeviceAdded or USB::onDeviceRemoved may > touch an unreachable object because after USB is declared unreachable an object > it refers to may be destroyed but it itself may not be? Maybe your USB object is destroyed before the frame gets detached? I think it can happen and it's totally valid.
On 2016/03/31 at 23:48:16, haraken wrote: > https://codereview.chromium.org/1848603002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/webusb/USB.cpp (right): > > https://codereview.chromium.org/1848603002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/webusb/USB.cpp:88: m_client->removeObserver(this); > On 2016/03/31 23:12:57, Reilly Grant wrote: > > On 2016/03/30 at 23:44:05, haraken wrote: > > > On 2016/03/30 23:38:10, Reilly Grant wrote: > > > > On 2016/03/30 at 23:35:23, haraken wrote: > > > > > Is it possible that USB's destructor is called without > > > > USB::willDetachFrameHost() getting called? > > > > > > > > USB::willDetachFrameHost() doesn't appear to be being called at all. Do you > > know > > > > why? It violates assumptions I had about LocalFrame lifetimes. > > > > > > It must be called. Is the USB object associated with a valid frame? > > > > I have tested again and confirmed that USB::willDetachFrameHost() is not called > > before USB::~USB(). How would I know if the frame was valid? The > > LocalFrameLifecycleObserver is initialized using the LocalFrame passed to > > USB::USB() which is Navigator::frame() and guaranteed to be non-null. > > > > > > > If that is possible, I think you need to make sure that > > > > m_client->removeObserver(this) is called before GC's sweeping phase starts. > > > > Otherwise, the following scenario can happen: > > > > > > > > > > 1) GC is triggered. USB becomes unreachable. > > > > > 2) Lazy sweeping starts. It destructs some unreachable object. Assume that > > USB > > > > isn't yet destructed. > > > > > 3) m_client accesses the already destructed object. > > > > > > > > > > To avoid that from happening, you need to make sure that > > > > m_client->removeObserver(this) is called before the sweeping phase starts. > > > > Specifically, you need to add USING_PRE_FINALIZER() and call > > > > m_client->removeObserver(this) in the pre-finalizer. > > > > Looking at this again I'm not sure I understand why a pre-finalizer is > > necessary. Are you saying that USB::onDeviceAdded or USB::onDeviceRemoved may > > touch an unreachable object because after USB is declared unreachable an object > > it refers to may be destroyed but it itself may not be? > > Maybe your USB object is destroyed before the frame gets detached? I think it can happen and it's totally valid. Thanks. I think I understand now. Added a pre-finalizer.
https://codereview.chromium.org/1848603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webusb/USB.h (right): https://codereview.chromium.org/1848603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webusb/USB.h:30: WILL_BE_USING_PRE_FINALIZER(USB, dispose); You need to register the pre-finalizer. Otherwise, it doesn't run. ThreadState::registerPreFinalizer(...)
https://codereview.chromium.org/1848603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webusb/USB.h (right): https://codereview.chromium.org/1848603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webusb/USB.h:30: WILL_BE_USING_PRE_FINALIZER(USB, dispose); On 2016/04/01 at 01:12:44, haraken wrote: > You need to register the pre-finalizer. Otherwise, it doesn't run. > > ThreadState::registerPreFinalizer(...) Done.
LGTM https://codereview.chromium.org/1848603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webusb/USB.cpp (right): https://codereview.chromium.org/1848603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webusb/USB.cpp:93: m_client->removeObserver(this); Shall we clear m_client here just in case? Then you can DCHECK(!m_client) in USB's destructor.
On 2016/04/01 at 01:36:06, haraken wrote: > LGTM > > https://codereview.chromium.org/1848603002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/webusb/USB.cpp (right): > > https://codereview.chromium.org/1848603002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webusb/USB.cpp:93: m_client->removeObserver(this); > > Shall we clear m_client here just in case? Then you can DCHECK(!m_client) in USB's destructor. Done.
The CQ bit was checked by reillyg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1848603002/#ps60001 (title: "DCHECK that m_client has been cleared before destruction.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1848603002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1848603002/60001
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/1848603002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webusb/USB.h (right): https://codereview.chromium.org/1848603002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webusb/USB.h:13: #include "platform/heap/ThreadState.h" Not needed. https://codereview.chromium.org/1848603002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webusb/USB.h:30: USING_PRE_FINALIZER(USB, dispose); This will work, but our convention has been to use EAGERLY_FINALIZE() rather than prefinalizers for simple objects that are clients/observers to embedder-side objects.
The CQ bit was unchecked by reillyg@chromium.org
On 2016/04/01 at 20:06:38, sigbjornf wrote: > https://codereview.chromium.org/1848603002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/webusb/USB.h (right): > > https://codereview.chromium.org/1848603002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webusb/USB.h:13: #include "platform/heap/ThreadState.h" > Not needed. > > https://codereview.chromium.org/1848603002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webusb/USB.h:30: USING_PRE_FINALIZER(USB, dispose); > This will work, but our convention has been to use EAGERLY_FINALIZE() rather than prefinalizers for simple objects that are clients/observers to embedder-side objects. Done.
The CQ bit was checked by reillyg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1848603002/#ps80001 (title: "Use EAGERLY_FINALIZE() instead.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1848603002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1848603002/80001
Note, this is a very temporary workaround as I will soon be removing the //content/renderer layer for WebUSB and this problem will go away.
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Remove single observer assumption in WebUSBClient. This patch removes the assumption in WebUSBClient that there will be a single instance of blink::USB active at once. Delayed garbage collection can mean that an old instance exists for some time after a new instance has added itself as an observer. With only a single observer slot the old instance could accidentally unregister the new instance when it was destroyed. BUG=598855 ========== to ========== Remove single observer assumption in WebUSBClient. This patch removes the assumption in WebUSBClient that there will be a single instance of blink::USB active at once. Delayed garbage collection can mean that an old instance exists for some time after a new instance has added itself as an observer. With only a single observer slot the old instance could accidentally unregister the new instance when it was destroyed. BUG=598855 Committed: https://crrev.com/e0584330d7c9ddf1887547d58cc2a7e0cb92fe1b Cr-Commit-Position: refs/heads/master@{#384763} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e0584330d7c9ddf1887547d58cc2a7e0cb92fe1b Cr-Commit-Position: refs/heads/master@{#384763}
Message was sent while issue was closed.
lgtm, thanks. https://codereview.chromium.org/1848603002/diff/80001/content/renderer/usb/we... File content/renderer/usb/web_usb_client_impl.cc (right): https://codereview.chromium.org/1848603002/diff/80001/content/renderer/usb/we... content/renderer/usb/web_usb_client_impl.cc:136: observers_.insert(observer); It sounds as if this code won't stay around for long, but.. would a DCHECK() on non-membership be worthwhile? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
