Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801403002:
MD Settings: Bluetooth: Fix adapter state and discovery (Closed)
|
Created:
3 years, 8 months ago by stevenjb Modified:
3 years, 8 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, chromium-apps-reviews_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Bluetooth: Fix adapter state and discovery
This CL:
* Adds some additional bluetooth event logging (USER events).
* Fixes the toggle logic so as not to set powered=false before the
adapter state is received or when the toggle state matches the
adapter state.
* Disables the toggle until the adapter state changes (not normally
observable, but can prevent rapid toggling artifacts).
* Modifies BluetoothEventRouter to stop discovery when all WebUI tabs
(or all tabs/windows associated with an extension) are closed.
BUG=703694, 703698
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2801403002
Cr-Commit-Position: refs/heads/master@{#464258}
Committed: https://chromium.googlesource.com/chromium/src/+/f4de1dce7be5bc5c6fcf4615478757773dace027
Patch Set 1 #Patch Set 2 : . #
Total comments: 8
Patch Set 3 : Improve comment #
Total comments: 5
Patch Set 4 : Correct comment and add more logging #Patch Set 5 : Track event router listeners by id #Patch Set 6 : Fix unittests #Patch Set 7 : Fix test #
Total comments: 14
Patch Set 8 : Feedback #Patch Set 9 : Use CHECK instead of early exit #Messages
Total messages: 44 (16 generated)
Description was changed from ========== MD Settings: Bluetooth: Fix adapter state and discovery This CL: * Adds some additional bluetooth event logging (USER events). * Fixes the toggle logic so as not to set powered=false before the adapter state is received or when the toggle state matches the adapter state. * Disables the toggle until the adapter state changes (not normally observable, but can prevent rapid toggling artifacts). * Modifies BluetoothEventRouter to stop discovery when all WebUI tabs (or all tabs/windows associated with an extension) are closed. BUG=703694,703698 ========== to ========== MD Settings: Bluetooth: Fix adapter state and discovery This CL: * Adds some additional bluetooth event logging (USER events). * Fixes the toggle logic so as not to set powered=false before the adapter state is received or when the toggle state matches the adapter state. * Disables the toggle until the adapter state changes (not normally observable, but can prevent rapid toggling artifacts). * Modifies BluetoothEventRouter to stop discovery when all WebUI tabs (or all tabs/windows associated with an extension) are closed. BUG=703694,703698 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
stevenjb@chromium.org changed reviewers: + michaelpg@chromium.org, rkc@chromium.org
stevenjb@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
+rdevlin.cronin@ for extensions/
//e/b/a/bluetooth* lgtm I'll leave the review for the settings changes to Michael.
https://codereview.chromium.org/2801403002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js (right): https://codereview.chromium.org/2801403002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:36: * Reflects the current state of the toggle buttons. This will be set when "button" singular? https://codereview.chromium.org/2801403002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:40: bluetoothToggleState_: { "state" is generally an unfortunate word, but hard to get around sometimes. In this case though, also having |adapterState_| of a completely different type makes me think it's worth finding a better name here. https://codereview.chromium.org/2801403002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:189: this.bluetoothToggleDisabled_ = true; I understand disabling at startup, but is disabling here necessary, and definitely not noticeable? https://codereview.chromium.org/2801403002/diff/20001/extensions/browser/api/... File extensions/browser/api/bluetooth/bluetooth_api.cc (right): https://codereview.chromium.org/2801403002/diff/20001/extensions/browser/api/... extensions/browser/api/bluetooth/bluetooth_api.cc:68: BLUETOOTH_LOG(EVENT) << "BluetoothAPI: " << browser_context_; is it normal to print a pointer?
https://codereview.chromium.org/2801403002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js (right): https://codereview.chromium.org/2801403002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:36: * Reflects the current state of the toggle buttons. This will be set when On 2017/04/07 21:47:03, michaelpg wrote: > "button" singular? There are two, one in this page and one in the subpage. I will clarify that. https://codereview.chromium.org/2801403002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:40: bluetoothToggleState_: { On 2017/04/07 21:47:03, michaelpg wrote: > "state" is generally an unfortunate word, but hard to get around sometimes. In > this case though, also having |adapterState_| of a completely different type > makes me think it's worth finding a better name here. I agree it isn't great, but there are not a lot of good options. 'Enabled' is worse, ambiguity wise. This represents the state of the toggle button, so I think 'toggle state' is reasonably OK. I am open to suggestions :) https://codereview.chromium.org/2801403002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:189: this.bluetoothToggleDisabled_ = true; On 2017/04/07 21:47:03, michaelpg wrote: > I understand disabling at startup, but is disabling here necessary, and > definitely not noticeable? We do the disable specifically for times when it is noticeable. If the user changes the state while a pending change is in progress it will be ignored. i.e. if the user clicks the button twice quickly they would see the following: on -> off off -> on (device change is in progress, so this is ignored) on -> off (when the device state changes to off) With the toggleDisabled state, the second click is ignored, so the user never sees the toggle flicker on. https://codereview.chromium.org/2801403002/diff/20001/extensions/browser/api/... File extensions/browser/api/bluetooth/bluetooth_api.cc (right): https://codereview.chromium.org/2801403002/diff/20001/extensions/browser/api/... extensions/browser/api/bluetooth/bluetooth_api.cc:68: BLUETOOTH_LOG(EVENT) << "BluetoothAPI: " << browser_context_; On 2017/04/07 21:47:03, michaelpg wrote: > is it normal to print a pointer? I don't know about 'normal' but we do it. It can be helpful for identifying which instance(s) are alive when there may be more than one.
https://codereview.chromium.org/2801403002/diff/40001/extensions/browser/api/... File extensions/browser/api/bluetooth/bluetooth_event_router.cc (right): https://codereview.chromium.org/2801403002/diff/40001/extensions/browser/api/... extensions/browser/api/bluetooth/bluetooth_event_router.cc:227: // When a tab is closed, OnExtensionUnloaded may not get called, so when This comment strikes me as strange - why would we expect OnExtensionUnloaded to be called when a tab closes?
https://codereview.chromium.org/2801403002/diff/40001/extensions/browser/api/... File extensions/browser/api/bluetooth/bluetooth_event_router.cc (right): https://codereview.chromium.org/2801403002/diff/40001/extensions/browser/api/... extensions/browser/api/bluetooth/bluetooth_event_router.cc:227: // When a tab is closed, OnExtensionUnloaded may not get called, so when On 2017/04/08 00:22:28, Devlin wrote: > This comment strikes me as strange - why would we expect OnExtensionUnloaded to > be called when a tab closes? The author assumed that it would (we call CleanUpForExtension() from OnExtensionUnloaded()). If it is the last tab/window running the extension, that seems potentially reasonable? Looking at the history, it appears that originally it was observing both NOTIFICATION_EXTENSION_UNLOADED and NOTIFICATION_EXTENSION_HOST_DESTROYED. "host destroyed" sounds more like what we actually want to observe. I think we probably actually want this to be a EventRouter::Observer. I will look into that.
https://codereview.chromium.org/2801403002/diff/40001/extensions/browser/api/... File extensions/browser/api/bluetooth/bluetooth_event_router.cc (right): https://codereview.chromium.org/2801403002/diff/40001/extensions/browser/api/... extensions/browser/api/bluetooth/bluetooth_event_router.cc:227: // When a tab is closed, OnExtensionUnloaded may not get called, so when On 2017/04/08 00:40:51, stevenjb wrote: > On 2017/04/08 00:22:28, Devlin wrote: > > This comment strikes me as strange - why would we expect OnExtensionUnloaded > to > > be called when a tab closes? > > The author assumed that it would (we call CleanUpForExtension() from > OnExtensionUnloaded()). If it is the last tab/window running the extension, that > seems potentially reasonable? > > Looking at the history, it appears that originally it was observing both > NOTIFICATION_EXTENSION_UNLOADED and NOTIFICATION_EXTENSION_HOST_DESTROYED. > > "host destroyed" sounds more like what we actually want to observe. I think we > probably actually want this to be a EventRouter::Observer. I will look into > that. > Tabs don't really "run" extensions, so assuming that the extension stops observing events when a tab is closed is pretty flawed. Watching for host destruction would probably work, but only coincidentally. Your hunch of EventRouter::Observer does indeed sound like what you want.
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/2801403002/diff/40001/extensions/browser/api/... File extensions/browser/api/bluetooth/bluetooth_event_router.cc (right): https://codereview.chromium.org/2801403002/diff/40001/extensions/browser/api/... extensions/browser/api/bluetooth/bluetooth_event_router.cc:227: // When a tab is closed, OnExtensionUnloaded may not get called, so when On 2017/04/08 00:43:43, Devlin wrote: > On 2017/04/08 00:40:51, stevenjb wrote: > > On 2017/04/08 00:22:28, Devlin wrote: > > > This comment strikes me as strange - why would we expect OnExtensionUnloaded > > to > > > be called when a tab closes? > > > > The author assumed that it would (we call CleanUpForExtension() from > > OnExtensionUnloaded()). If it is the last tab/window running the extension, > that > > seems potentially reasonable? > > > > Looking at the history, it appears that originally it was observing both > > NOTIFICATION_EXTENSION_UNLOADED and NOTIFICATION_EXTENSION_HOST_DESTROYED. > > > > "host destroyed" sounds more like what we actually want to observe. I think we > > probably actually want this to be a EventRouter::Observer. I will look into > > that. > > > > Tabs don't really "run" extensions, so assuming that the extension stops > observing events when a tab is closed is pretty flawed. Watching for host > destruction would probably work, but only coincidentally. Your hunch of > EventRouter::Observer does indeed sound like what you want. OK, so, I added some more logging so I have a better idea of what is going on here and can improve the comment. It turns out that we are receiving NOTIFICATION_EXTENSION_HOST_DESTROYED events, just not for WebUI, which makes sense. So, currently the only way this code knows that WebUI may be detached is here. I couldn't find any other straightforward way to detect this, and since we are releasing the adapter, cleaning up any discovery and filter maps is a good idea anyway.
lgtm https://codereview.chromium.org/2801403002/diff/40001/extensions/browser/api/... File extensions/browser/api/bluetooth/bluetooth_event_router.cc (right): https://codereview.chromium.org/2801403002/diff/40001/extensions/browser/api/... extensions/browser/api/bluetooth/bluetooth_event_router.cc:227: // When a tab is closed, OnExtensionUnloaded may not get called, so when On 2017/04/08 02:26:15, stevenjb wrote: > On 2017/04/08 00:43:43, Devlin wrote: > > On 2017/04/08 00:40:51, stevenjb wrote: > > > On 2017/04/08 00:22:28, Devlin wrote: > > > > This comment strikes me as strange - why would we expect > OnExtensionUnloaded > > > to > > > > be called when a tab closes? > > > > > > The author assumed that it would (we call CleanUpForExtension() from > > > OnExtensionUnloaded()). If it is the last tab/window running the extension, > > that > > > seems potentially reasonable? > > > > > > Looking at the history, it appears that originally it was observing both > > > NOTIFICATION_EXTENSION_UNLOADED and NOTIFICATION_EXTENSION_HOST_DESTROYED. > > > > > > "host destroyed" sounds more like what we actually want to observe. I think > we > > > probably actually want this to be a EventRouter::Observer. I will look into > > > that. > > > > > > > Tabs don't really "run" extensions, so assuming that the extension stops > > observing events when a tab is closed is pretty flawed. Watching for host > > destruction would probably work, but only coincidentally. Your hunch of > > EventRouter::Observer does indeed sound like what you want. > > OK, so, I added some more logging so I have a better idea of what is going on > here and can improve the comment. > > It turns out that we are receiving NOTIFICATION_EXTENSION_HOST_DESTROYED events, > just not for WebUI, which makes sense. > > So, currently the only way this code knows that WebUI may be detached is here. I > couldn't find any other straightforward way to detect this, and since we are > releasing the adapter, cleaning up any discovery and filter maps is a good idea > anyway. This still doesn't quite make sense to me, but I'll defer to you as the bluetooth owner.
I was less than pleased with the previous heavy handed solution so I did some more digging and came up with a somewhat better solution for identifying when to stop discovery and pairing when WebUI tab is closed. PTAL
The CQ bit was checked by stevenjb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
rdevlin.cronin@, rkc@ PTAL delta from PS 4.
The CQ bit was checked by stevenjb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
rdevlin.cronin@, rkc@ could you PTAL delta from PS 4. This is a P1 for M59.
lgtm % comment https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... File extensions/browser/api/bluetooth/bluetooth_event_router_unittest.cc (right): https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... extensions/browser/api/bluetooth/bluetooth_event_router_unittest.cc:70: EventListenerInfo info("", "", GURL(), nullptr); Can we expand this test (or add additional tests) to test for more combinations of URLs and extension ID's?
https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... File extensions/browser/api/bluetooth/bluetooth_event_router_unittest.cc (right): https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... extensions/browser/api/bluetooth/bluetooth_event_router_unittest.cc:70: EventListenerInfo info("", "", GURL(), nullptr); On 2017/04/12 17:58:52, rkc wrote: > Can we expand this test (or add additional tests) to test for more combinations > of URLs and extension ID's? It would be a fair bit of work to add a meaningful test for multiple extensions and WebUI host I think, and frankly I would want to significantly refactor this code and test the refactor. However, I'm not really in a position to take ownership of this code just now...
That's fine - but then please do file a bug for expanding this test and link it in a TODO here. Feel free to assign the bug to me. I'll see if I can find an engineer to pick it up.
https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... File extensions/browser/api/bluetooth/bluetooth_event_router.cc (right): https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... extensions/browser/api/bluetooth/bluetooth_event_router.cc:50: : details.listener_url.host(); Haven't we had issues with using the host as an id before? Does this handle e.g. multiple tabs with the same host? https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... extensions/browser/api/bluetooth/bluetooth_event_router.cc:390: return; // Edge case if adapter wasn't ready when listener was added. If this can happen, doesn't that mean we could also be off-by-one somewhere? https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... extensions/browser/api/bluetooth/bluetooth_event_router.cc:394: if (count <= 0) { < 0? How?
https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... File extensions/browser/api/bluetooth/bluetooth_event_router.cc (right): https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... extensions/browser/api/bluetooth/bluetooth_event_router.cc:50: : details.listener_url.host(); On 2017/04/12 19:21:44, Devlin wrote: > Haven't we had issues with using the host as an id before? Does this handle > e.g. multiple tabs with the same host? The code (and TODO) you are thinking of (I assume) are at the top of event_listener_map.cc, We already call url::Origin(listener_url).GetURL(). This just extracts the relevant part as a string (we also do this in bluetooth_private_api.cc). I want to replace 'extension_id' and 'url' with 'listener_id' in EventListenerInfo so this isn't needed, but that is more involved and we should do that separately. https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... extensions/browser/api/bluetooth/bluetooth_event_router.cc:390: return; // Edge case if adapter wasn't ready when listener was added. On 2017/04/12 19:21:44, Devlin wrote: > If this can happen, doesn't that mean we could also be off-by-one somewhere? We can theoretically (by code inspection) remove more than add but not vice versa. I don't think it can happen in practice. Updated comment. https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... extensions/browser/api/bluetooth/bluetooth_event_router.cc:394: if (count <= 0) { On 2017/04/12 19:21:44, Devlin wrote: > < 0? How? Habit, changed to == 0.
PTAL
https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... File extensions/browser/api/bluetooth/bluetooth_event_router.cc (right): https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... extensions/browser/api/bluetooth/bluetooth_event_router.cc:390: return; // Edge case if adapter wasn't ready when listener was added. On 2017/04/12 20:35:41, stevenjb wrote: > On 2017/04/12 19:21:44, Devlin wrote: > > If this can happen, doesn't that mean we could also be off-by-one somewhere? > > We can theoretically (by code inspection) remove more than add but not vice > versa. I don't think it can happen in practice. Updated comment. > So, my interpretation of your comment here is that the following is possible: 1. Listener added 2. Adapter initialized 3. Listener removed -> no listener entry But, doesn't that mean we could also have: 1. Listener added 2. Adapter initialized 3. Listener 2 added 4. Listener removed -> we'll remove the entry in the map, despite the fact that there's still an active listener. Am I missing something?
https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... File extensions/browser/api/bluetooth/bluetooth_event_router.cc (right): https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... extensions/browser/api/bluetooth/bluetooth_event_router.cc:390: return; // Edge case if adapter wasn't ready when listener was added. On 2017/04/12 22:23:20, Devlin wrote: > On 2017/04/12 20:35:41, stevenjb wrote: > > On 2017/04/12 19:21:44, Devlin wrote: > > > If this can happen, doesn't that mean we could also be off-by-one somewhere? > > > > We can theoretically (by code inspection) remove more than add but not vice > > versa. I don't think it can happen in practice. Updated comment. > > > > So, my interpretation of your comment here is that the following is possible: > > 1. Listener added > 2. Adapter initialized > 3. Listener removed -> no listener entry > > But, doesn't that mean we could also have: > 1. Listener added > 2. Adapter initialized > 3. Listener 2 added > 4. Listener removed -> we'll remove the entry in the map, despite the fact that > there's still an active listener. > > Am I missing something? 1. Listener is *not* added because adapter is not intialized: https://cs.chromium.org/chromium/src/extensions/browser/api/bluetooth/bluetoo...
https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... File extensions/browser/api/bluetooth/bluetooth_event_router.cc (right): https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... extensions/browser/api/bluetooth/bluetooth_event_router.cc:390: return; // Edge case if adapter wasn't ready when listener was added. On 2017/04/12 22:33:01, stevenjb wrote: > On 2017/04/12 22:23:20, Devlin wrote: > > On 2017/04/12 20:35:41, stevenjb wrote: > > > On 2017/04/12 19:21:44, Devlin wrote: > > > > If this can happen, doesn't that mean we could also be off-by-one > somewhere? > > > > > > We can theoretically (by code inspection) remove more than add but not vice > > > versa. I don't think it can happen in practice. Updated comment. > > > > > > > So, my interpretation of your comment here is that the following is possible: > > > > 1. Listener added > > 2. Adapter initialized > > 3. Listener removed -> no listener entry > > > > But, doesn't that mean we could also have: > > 1. Listener added > > 2. Adapter initialized > > 3. Listener 2 added > > 4. Listener removed -> we'll remove the entry in the map, despite the fact > that > > there's still an active listener. > > > > Am I missing something? > > 1. Listener is *not* added because adapter is not intialized: > https://cs.chromium.org/chromium/src/extensions/browser/api/bluetooth/bluetoo... I should have been more clear. 1. Listener added through addListener(), notifies EventRouter, notifies BluetoothAPI, checks adapter - adapter unitialized. Net State: 1 listener in EventRouter 0 listeners in BluetoothEventRouter 2. Adapter initialized Net State: 1 listener in EventRouter 0 listeners in BluetoothEventRouter 3. Listener 2 added through addListener(), notifies EventRouter, notifies BluetoothAPI, adapter is initialized, so notifies BluetoothEventRouter. Net State: 2 listeners in EventRouter 1 listener in BluetoothEventRouter 4. Listener removed through removeListener(), notifies EventRouter, notifies BluetoothAPI, adapter is initialized, so notifies BluetoothEventRouter. We'll remove the entry in the map, despite the fact that there's still an active listener in EventRouter. Net State: 1 listener in EventRouter 0 listeners in BluetoothEventRouter
https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... File extensions/browser/api/bluetooth/bluetooth_event_router.cc (right): https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... extensions/browser/api/bluetooth/bluetooth_event_router.cc:390: return; // Edge case if adapter wasn't ready when listener was added. On 2017/04/12 23:01:26, Devlin wrote: > On 2017/04/12 22:33:01, stevenjb wrote: > > On 2017/04/12 22:23:20, Devlin wrote: > > > On 2017/04/12 20:35:41, stevenjb wrote: > > > > On 2017/04/12 19:21:44, Devlin wrote: > > > > > If this can happen, doesn't that mean we could also be off-by-one > > somewhere? > > > > > > > > We can theoretically (by code inspection) remove more than add but not > vice > > > > versa. I don't think it can happen in practice. Updated comment. > > > > > > > > > > So, my interpretation of your comment here is that the following is > possible: > > > > > > 1. Listener added > > > 2. Adapter initialized > > > 3. Listener removed -> no listener entry > > > > > > But, doesn't that mean we could also have: > > > 1. Listener added > > > 2. Adapter initialized > > > 3. Listener 2 added > > > 4. Listener removed -> we'll remove the entry in the map, despite the fact > > that > > > there's still an active listener. > > > > > > Am I missing something? > > > > 1. Listener is *not* added because adapter is not intialized: > > > https://cs.chromium.org/chromium/src/extensions/browser/api/bluetooth/bluetoo... > > I should have been more clear. > > 1. Listener added through addListener(), notifies EventRouter, notifies > BluetoothAPI, checks adapter - adapter unitialized. > > Net State: > 1 listener in EventRouter > 0 listeners in BluetoothEventRouter > > 2. Adapter initialized > > Net State: > 1 listener in EventRouter > 0 listeners in BluetoothEventRouter > > 3. Listener 2 added through addListener(), notifies EventRouter, notifies > BluetoothAPI, adapter is initialized, so notifies BluetoothEventRouter. > > Net State: > 2 listeners in EventRouter > 1 listener in BluetoothEventRouter > > 4. Listener removed through removeListener(), notifies EventRouter, notifies > BluetoothAPI, adapter is initialized, so notifies BluetoothEventRouter. We'll > remove the entry in the map, despite the fact that there's still an active > listener in EventRouter. > > Net State: > 1 listener in EventRouter > 0 listeners in BluetoothEventRouter HUH???? "added through addListener()" - what addListener where? Basically: In this code: https://cs.chromium.org/chromium/src/extensions/browser/api/bluetooth/bluetoo... We may not call addListener for "Reasons" that I think never happen but, in theory, could. Therefore we could, in theory, call this method (OnListenerRemoved) without a corresponding call to OnListenerAdded(). The opposite may or may not be true but I don't care about that - it won't cause a crash, just another dangling ref counter and this code is so full of potential for that I barely know where to begin. I can put a CHECK here instead. What would you like me to do, other than rewrite all of this code which I simply do not have time for?
lgtm again with a CHECK. Sorry for the back-and-forth. https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... File extensions/browser/api/bluetooth/bluetooth_event_router.cc (right): https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... extensions/browser/api/bluetooth/bluetooth_event_router.cc:390: return; // Edge case if adapter wasn't ready when listener was added. On 2017/04/12 23:12:28, stevenjb wrote: > On 2017/04/12 23:01:26, Devlin wrote: > > On 2017/04/12 22:33:01, stevenjb wrote: > > > On 2017/04/12 22:23:20, Devlin wrote: > > > > On 2017/04/12 20:35:41, stevenjb wrote: > > > > > On 2017/04/12 19:21:44, Devlin wrote: > > > > > > If this can happen, doesn't that mean we could also be off-by-one > > > somewhere? > > > > > > > > > > We can theoretically (by code inspection) remove more than add but not > > vice > > > > > versa. I don't think it can happen in practice. Updated comment. > > > > > > > > > > > > > So, my interpretation of your comment here is that the following is > > possible: > > > > > > > > 1. Listener added > > > > 2. Adapter initialized > > > > 3. Listener removed -> no listener entry > > > > > > > > But, doesn't that mean we could also have: > > > > 1. Listener added > > > > 2. Adapter initialized > > > > 3. Listener 2 added > > > > 4. Listener removed -> we'll remove the entry in the map, despite the fact > > > that > > > > there's still an active listener. > > > > > > > > Am I missing something? > > > > > > 1. Listener is *not* added because adapter is not intialized: > > > > > > https://cs.chromium.org/chromium/src/extensions/browser/api/bluetooth/bluetoo... > > > > I should have been more clear. > > > > 1. Listener added through addListener(), notifies EventRouter, notifies > > BluetoothAPI, checks adapter - adapter unitialized. > > > > Net State: > > 1 listener in EventRouter > > 0 listeners in BluetoothEventRouter > > > > 2. Adapter initialized > > > > Net State: > > 1 listener in EventRouter > > 0 listeners in BluetoothEventRouter > > > > 3. Listener 2 added through addListener(), notifies EventRouter, notifies > > BluetoothAPI, adapter is initialized, so notifies BluetoothEventRouter. > > > > Net State: > > 2 listeners in EventRouter > > 1 listener in BluetoothEventRouter > > > > 4. Listener removed through removeListener(), notifies EventRouter, notifies > > BluetoothAPI, adapter is initialized, so notifies BluetoothEventRouter. We'll > > remove the entry in the map, despite the fact that there's still an active > > listener in EventRouter. > > > > Net State: > > 1 listener in EventRouter > > 0 listeners in BluetoothEventRouter > > HUH???? > > "added through addListener()" - what addListener where? > > Basically: > > In this code: > https://cs.chromium.org/chromium/src/extensions/browser/api/bluetooth/bluetoo... > > We may not call addListener for "Reasons" that I think never happen but, in > theory, could. > > Therefore we could, in theory, call this method (OnListenerRemoved) without a > corresponding call to OnListenerAdded(). > > The opposite may or may not be true but I don't care about that - it won't cause > a crash, just another dangling ref counter and this code is so full of potential > for that I barely know where to begin. > > I can put a CHECK here instead. > > What would you like me to do, other than rewrite all of this code which I simply > do not have time for? > addListener() == the JS addListener function, which is forwarded to the event router. Making this CHECK would solve my concerns. :)
Thanks for the review! Hopefully someone will have a chance to clean this all up sooner than later. https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... File extensions/browser/api/bluetooth/bluetooth_event_router.cc (right): https://codereview.chromium.org/2801403002/diff/140001/extensions/browser/api... extensions/browser/api/bluetooth/bluetooth_event_router.cc:390: return; // Edge case if adapter wasn't ready when listener was added. On 2017/04/13 00:13:39, Devlin wrote: > On 2017/04/12 23:12:28, stevenjb wrote: > > On 2017/04/12 23:01:26, Devlin wrote: > > > On 2017/04/12 22:33:01, stevenjb wrote: > > > > On 2017/04/12 22:23:20, Devlin wrote: > > > > > On 2017/04/12 20:35:41, stevenjb wrote: > > > > > > On 2017/04/12 19:21:44, Devlin wrote: > > > > > > > If this can happen, doesn't that mean we could also be off-by-one > > > > somewhere? > > > > > > > > > > > > We can theoretically (by code inspection) remove more than add but not > > > vice > > > > > > versa. I don't think it can happen in practice. Updated comment. > > > > > > > > > > > > > > > > So, my interpretation of your comment here is that the following is > > > possible: > > > > > > > > > > 1. Listener added > > > > > 2. Adapter initialized > > > > > 3. Listener removed -> no listener entry > > > > > > > > > > But, doesn't that mean we could also have: > > > > > 1. Listener added > > > > > 2. Adapter initialized > > > > > 3. Listener 2 added > > > > > 4. Listener removed -> we'll remove the entry in the map, despite the > fact > > > > that > > > > > there's still an active listener. > > > > > > > > > > Am I missing something? > > > > > > > > 1. Listener is *not* added because adapter is not intialized: > > > > > > > > > > https://cs.chromium.org/chromium/src/extensions/browser/api/bluetooth/bluetoo... > > > > > > I should have been more clear. > > > > > > 1. Listener added through addListener(), notifies EventRouter, notifies > > > BluetoothAPI, checks adapter - adapter unitialized. > > > > > > Net State: > > > 1 listener in EventRouter > > > 0 listeners in BluetoothEventRouter > > > > > > 2. Adapter initialized > > > > > > Net State: > > > 1 listener in EventRouter > > > 0 listeners in BluetoothEventRouter > > > > > > 3. Listener 2 added through addListener(), notifies EventRouter, notifies > > > BluetoothAPI, adapter is initialized, so notifies BluetoothEventRouter. > > > > > > Net State: > > > 2 listeners in EventRouter > > > 1 listener in BluetoothEventRouter > > > > > > 4. Listener removed through removeListener(), notifies EventRouter, notifies > > > BluetoothAPI, adapter is initialized, so notifies BluetoothEventRouter. > We'll > > > remove the entry in the map, despite the fact that there's still an active > > > listener in EventRouter. > > > > > > Net State: > > > 1 listener in EventRouter > > > 0 listeners in BluetoothEventRouter > > > > HUH???? > > > > "added through addListener()" - what addListener where? > > > > Basically: > > > > In this code: > > > https://cs.chromium.org/chromium/src/extensions/browser/api/bluetooth/bluetoo... > > > > We may not call addListener for "Reasons" that I think never happen but, in > > theory, could. > > > > Therefore we could, in theory, call this method (OnListenerRemoved) without a > > corresponding call to OnListenerAdded(). > > > > The opposite may or may not be true but I don't care about that - it won't > cause > > a crash, just another dangling ref counter and this code is so full of > potential > > for that I barely know where to begin. > > > > I can put a CHECK here instead. > > > > What would you like me to do, other than rewrite all of this code which I > simply > > do not have time for? > > > > addListener() == the JS addListener function, which is forwarded to the event > router. > > Making this CHECK would solve my concerns. :) Done.
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelpg@chromium.org, rkc@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2801403002/#ps180001 (title: "Use CHECK instead of early exit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1492043149389710,
"parent_rev": "bc40dc02825d205b8fa33c618026d4c639bb77c4", "commit_rev":
"f4de1dce7be5bc5c6fcf4615478757773dace027"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Bluetooth: Fix adapter state and discovery This CL: * Adds some additional bluetooth event logging (USER events). * Fixes the toggle logic so as not to set powered=false before the adapter state is received or when the toggle state matches the adapter state. * Disables the toggle until the adapter state changes (not normally observable, but can prevent rapid toggling artifacts). * Modifies BluetoothEventRouter to stop discovery when all WebUI tabs (or all tabs/windows associated with an extension) are closed. BUG=703694,703698 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Bluetooth: Fix adapter state and discovery This CL: * Adds some additional bluetooth event logging (USER events). * Fixes the toggle logic so as not to set powered=false before the adapter state is received or when the toggle state matches the adapter state. * Disables the toggle until the adapter state changes (not normally observable, but can prevent rapid toggling artifacts). * Modifies BluetoothEventRouter to stop discovery when all WebUI tabs (or all tabs/windows associated with an extension) are closed. BUG=703694,703698 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2801403002 Cr-Commit-Position: refs/heads/master@{#464258} Committed: https://chromium.googlesource.com/chromium/src/+/f4de1dce7be5bc5c6fcf46154787... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/f4de1dce7be5bc5c6fcf46154787... |
