Issue metadata
Sign in to add a comment
|
Files app stops responding to disk mount events. |
||||||||||||||||||||||
Issue descriptionChrome Version: 70.0.3505.0 OS: Chrome What steps will reproduce the problem? 1. Open 2 Files.app windows. 2. Plug in a USB flash drive containing a single mountable partition and wait for chrome to mount it. 3. Click the eject icon for the USB drive and close one of the two windows. 4. Unplug and re-plug in flash drive. What is the expected result? Files.app sees USB drives on subsequent plug-ins. What happens instead? Files.app doesn't see any USB drives when you plug them in. Initial digging shows the fileSystemPrivate.onMountCompleted listener being unregistered in the browser process EventRouter.
,
Jul 31
Based on debugging, I've narrowed down the repro. Step 2 is unnecessary. Repro steps: 1. Open 2 Files.app windows, close one of them. 2. Plug in a USB drive. The issue is a regression with the new native extensions system bindings. Disabling the "NativeCrxBindings" feature "fixes" the bug. To repro, you must be in the "NativeCrxBindings" field trail (at 50% dev/canary). I believe the issue is that each window (and the background page) has it's own Blink page and JS context. When one of those contexts is destroyed (when a window is closed), the renderer tells the browser to stop listening to events that that page has registered for. In the Files.app, some events are listened to by both the background page, and the app windows. Mount events are one of these. This prevents the background page from getting mount events. Stack trace for the renderer process (logging added): [1:1:0731/101319.223611:ERROR:native_extension_bindings_system.cc(791)] NativeExtensionBindingsSystem::OnEventListenerChanged remove storage.onChanged [1:1:0731/101319.223708:ERROR:ipc_message_sender.cc(65)] MT::SendRemoveUnfilteredEventListenerIPC storage.onChanged #0 0x563ec44be0fc base::debug::StackTrace::StackTrace() #1 0x563ec40b6d3e extensions::(anonymous namespace)::MainThreadIPCMessageSender::SendRemoveUnfilteredEventListenerIPC() #2 0x563ec40c2b3c extensions::NativeExtensionBindingsSystem::OnEventListenerChanged() #3 0x563ec4080c39 _ZN4base8internal7InvokerINS0_9BindStateINS_17RepeatingCallbackIFvRKNSt3__112basic_stringIcNS4_11char_traitsIcEENS4_9allocatorIcEEEEN10extensions7binding21EventListenersChangedEPKNS_15DictionaryValueEbN2v85LocalINSJ_7ContextEEEEEEJSA_EEEFvSF_SI_bSM_EE3RunEPNS0_13BindStateBaseESF_SI_bOSM_ #4 0x563ec4081484 extensions::UnfilteredEventListeners::Invalidate() #5 0x563ec408090f extensions::APIEventHandler::InvalidateContext() #6 0x563ec40c348c extensions::NativeExtensionBindingsSystem::WillReleaseScriptContext() #7 0x563ec4097076 extensions::Dispatcher::WillReleaseScriptContext() #8 0x563ec40a5de3 extensions::ExtensionFrameHelper::WillReleaseScriptContext() #9 0x563ec76635d7 content::RenderFrameImpl::WillReleaseScriptContext() #10 0x563ec82186c3 blink::LocalWindowProxy::DisposeContext() #11 0x563ec81e4f03 blink::WindowProxyManager::ClearForClose() #12 0x563ec81c7ed2 blink::ScriptController::ClearForClose() #13 0x563ec6fac1ac blink::LocalFrame::Detach() #14 0x563ec736b1fd blink::Page::WillBeDestroyed() #15 0x563ec6f5d6e9 blink::WebViewImpl::Close() #16 0x563ec7698aec content::RenderWidget::Close() #17 0x563ec44d2249 base::debug::TaskAnnotator::RunTask() #18 0x563ec4484e82 base::sequence_manager::internal::ThreadControllerImpl::DoWork() #19 0x563ec44d2249 base::debug::TaskAnnotator::RunTask() #20 0x563ec4445792 base::MessageLoop::RunTask() #21 0x563ec4445c67 base::MessageLoop::DoWork() #22 0x563ec4446c9a base::MessagePumpDefault::Run() #23 0x563ec4464d75 base::RunLoop::Run() #24 0x563ec76a1a3a content::RendererMain() #25 0x563ec410f1a7 content::RunZygote() #26 0x563ec4110606 content::ContentMainRunnerImpl::Run() #27 0x563ec411775c service_manager::Main() #28 0x563ec410e761 content::ContentMain() #29 0x563ec1f248ff ChromeMain #30 0x75afdf398736 __libc_start_main
,
Aug 10
I'm looking into this.
,
Aug 13
,
Aug 13
Issue 872959 has been merged into this issue.
,
Aug 14
,
Aug 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d2389080765a5086ff06355d8af16b35d54a4d69 commit d2389080765a5086ff06355d8af16b35d54a4d69 Author: Devlin Cronin <rdevlin.cronin@chromium.org> Date: Tue Aug 14 01:53:26 2018 [Extensions Bindings] Properly report added/removed unfiltered listeners Currently, native bindings will send an add/remove unfiltered listener IPC when the first listener is added and last listener is removed from a given v8::Context/ScriptContext, whereas JS-based bindings will report for the first/last listener for a given context owner (e.g., extension). This causes an issue in native bindings, since the browser side will not add duplicate listeners. Thus, if an extension adds two listeners and then removes one, the browser side will remove the listener registration for that extension. Fix this by only reporting when the first/last listener is added/removed for a given context owner with native bindings as well, except for lazy contexts (where a separate IPC needs to be sent to add/remove the lazy listener). Introduce a ListenerTracker to keep track of all active listeners across contexts, given a context owner. Use the ListenerTracker in the native bindings implementation; a follow-up will use the ListenerTracker in JS-based bindings as well (replacing the EventBookkeeper, which the ListenerTracker was heavily based on). Bug: 868763 Change-Id: I6bb80f48973fb84c7cce1c702d667023dac079ba Reviewed-on: https://chromium-review.googlesource.com/1166597 Commit-Queue: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org> Cr-Commit-Position: refs/heads/master@{#582793} [modify] https://crrev.com/d2389080765a5086ff06355d8af16b35d54a4d69/chrome/browser/extensions/extension_bindings_apitest.cc [modify] https://crrev.com/d2389080765a5086ff06355d8af16b35d54a4d69/extensions/renderer/BUILD.gn [modify] https://crrev.com/d2389080765a5086ff06355d8af16b35d54a4d69/extensions/renderer/bindings/api_binding_types.h [modify] https://crrev.com/d2389080765a5086ff06355d8af16b35d54a4d69/extensions/renderer/bindings/api_binding_unittest.cc [modify] https://crrev.com/d2389080765a5086ff06355d8af16b35d54a4d69/extensions/renderer/bindings/api_bindings_system.cc [modify] https://crrev.com/d2389080765a5086ff06355d8af16b35d54a4d69/extensions/renderer/bindings/api_bindings_system.h [modify] https://crrev.com/d2389080765a5086ff06355d8af16b35d54a4d69/extensions/renderer/bindings/api_bindings_system_unittest.cc [modify] https://crrev.com/d2389080765a5086ff06355d8af16b35d54a4d69/extensions/renderer/bindings/api_event_handler.cc [modify] https://crrev.com/d2389080765a5086ff06355d8af16b35d54a4d69/extensions/renderer/bindings/api_event_handler.h [modify] https://crrev.com/d2389080765a5086ff06355d8af16b35d54a4d69/extensions/renderer/bindings/api_event_handler_unittest.cc [modify] https://crrev.com/d2389080765a5086ff06355d8af16b35d54a4d69/extensions/renderer/bindings/api_event_listeners.cc [modify] https://crrev.com/d2389080765a5086ff06355d8af16b35d54a4d69/extensions/renderer/bindings/api_event_listeners.h [modify] https://crrev.com/d2389080765a5086ff06355d8af16b35d54a4d69/extensions/renderer/bindings/api_event_listeners_unittest.cc [modify] https://crrev.com/d2389080765a5086ff06355d8af16b35d54a4d69/extensions/renderer/bindings/event_emitter_unittest.cc [add] https://crrev.com/d2389080765a5086ff06355d8af16b35d54a4d69/extensions/renderer/bindings/listener_tracker.cc [add] https://crrev.com/d2389080765a5086ff06355d8af16b35d54a4d69/extensions/renderer/bindings/listener_tracker.h [add] https://crrev.com/d2389080765a5086ff06355d8af16b35d54a4d69/extensions/renderer/bindings/listener_tracker_unittest.cc [modify] https://crrev.com/d2389080765a5086ff06355d8af16b35d54a4d69/extensions/renderer/gin_port_unittest.cc [modify] https://crrev.com/d2389080765a5086ff06355d8af16b35d54a4d69/extensions/renderer/native_extension_bindings_system.cc
,
Aug 14
This should be fixed with #7. amistry@, since you were able to repro, could you double-check when you get a chance that all is well? We'll likely want to merge this to M69. I'll let it bake on Canary for a day and then request merge if all looks good.
,
Aug 14
,
Aug 14
,
Aug 15
The NextAction date has arrived: 2018-08-15
,
Aug 16
Still waiting for a build that picks this up.
,
Aug 20
I've verified this fix on 70.0.3524.2 and also verified this fixes crbug.com/873411, crbug.com/872950, and crbug.com/850654 .
,
Aug 20
Requesting merge to M69. This fixes some pretty gnarly bugs related to the files app with the NativeCrxBindings enabled (currently rolled out to beta). We'd like to avoid rolling the experiment back if we can, and this change only affects the experiment group (thus there should be no risk of a stable regression). It has baked on Canary for the better part of a week, and has caused no issues we know of.
,
Aug 20
This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 20
,
Aug 20
Thanks for the details in #14, merge approved M69.
,
Aug 20
Thanks, cindyb@! Can you confirm the branch number to merge into? (3497?)
,
Aug 22
The NextAction date has arrived: 2018-08-22
,
Aug 22
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8bda35334e83cbc1126aa5a683ffe6d89be4b3e4 commit 8bda35334e83cbc1126aa5a683ffe6d89be4b3e4 Author: Devlin Cronin <rdevlin.cronin@chromium.org> Date: Wed Aug 22 16:04:09 2018 [Extensions Bindings] Properly report added/removed unfiltered listeners Currently, native bindings will send an add/remove unfiltered listener IPC when the first listener is added and last listener is removed from a given v8::Context/ScriptContext, whereas JS-based bindings will report for the first/last listener for a given context owner (e.g., extension). This causes an issue in native bindings, since the browser side will not add duplicate listeners. Thus, if an extension adds two listeners and then removes one, the browser side will remove the listener registration for that extension. Fix this by only reporting when the first/last listener is added/removed for a given context owner with native bindings as well, except for lazy contexts (where a separate IPC needs to be sent to add/remove the lazy listener). Introduce a ListenerTracker to keep track of all active listeners across contexts, given a context owner. Use the ListenerTracker in the native bindings implementation; a follow-up will use the ListenerTracker in JS-based bindings as well (replacing the EventBookkeeper, which the ListenerTracker was heavily based on). Bug: 868763 TBR=rdevlin.cronin@chromium.org (cherry picked from commit d2389080765a5086ff06355d8af16b35d54a4d69) Change-Id: I6bb80f48973fb84c7cce1c702d667023dac079ba Reviewed-on: https://chromium-review.googlesource.com/1166597 Commit-Queue: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#582793} Reviewed-on: https://chromium-review.googlesource.com/1185222 Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#767} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/8bda35334e83cbc1126aa5a683ffe6d89be4b3e4/chrome/browser/extensions/extension_bindings_apitest.cc [modify] https://crrev.com/8bda35334e83cbc1126aa5a683ffe6d89be4b3e4/extensions/renderer/BUILD.gn [modify] https://crrev.com/8bda35334e83cbc1126aa5a683ffe6d89be4b3e4/extensions/renderer/bindings/api_binding_types.h [modify] https://crrev.com/8bda35334e83cbc1126aa5a683ffe6d89be4b3e4/extensions/renderer/bindings/api_binding_unittest.cc [modify] https://crrev.com/8bda35334e83cbc1126aa5a683ffe6d89be4b3e4/extensions/renderer/bindings/api_bindings_system.cc [modify] https://crrev.com/8bda35334e83cbc1126aa5a683ffe6d89be4b3e4/extensions/renderer/bindings/api_bindings_system.h [modify] https://crrev.com/8bda35334e83cbc1126aa5a683ffe6d89be4b3e4/extensions/renderer/bindings/api_bindings_system_unittest.cc [modify] https://crrev.com/8bda35334e83cbc1126aa5a683ffe6d89be4b3e4/extensions/renderer/bindings/api_event_handler.cc [modify] https://crrev.com/8bda35334e83cbc1126aa5a683ffe6d89be4b3e4/extensions/renderer/bindings/api_event_handler.h [modify] https://crrev.com/8bda35334e83cbc1126aa5a683ffe6d89be4b3e4/extensions/renderer/bindings/api_event_handler_unittest.cc [modify] https://crrev.com/8bda35334e83cbc1126aa5a683ffe6d89be4b3e4/extensions/renderer/bindings/api_event_listeners.cc [modify] https://crrev.com/8bda35334e83cbc1126aa5a683ffe6d89be4b3e4/extensions/renderer/bindings/api_event_listeners.h [modify] https://crrev.com/8bda35334e83cbc1126aa5a683ffe6d89be4b3e4/extensions/renderer/bindings/api_event_listeners_unittest.cc [modify] https://crrev.com/8bda35334e83cbc1126aa5a683ffe6d89be4b3e4/extensions/renderer/bindings/event_emitter_unittest.cc [add] https://crrev.com/8bda35334e83cbc1126aa5a683ffe6d89be4b3e4/extensions/renderer/bindings/listener_tracker.cc [add] https://crrev.com/8bda35334e83cbc1126aa5a683ffe6d89be4b3e4/extensions/renderer/bindings/listener_tracker.h [add] https://crrev.com/8bda35334e83cbc1126aa5a683ffe6d89be4b3e4/extensions/renderer/bindings/listener_tracker_unittest.cc [modify] https://crrev.com/8bda35334e83cbc1126aa5a683ffe6d89be4b3e4/extensions/renderer/gin_port_unittest.cc [modify] https://crrev.com/8bda35334e83cbc1126aa5a683ffe6d89be4b3e4/extensions/renderer/native_extension_bindings_system.cc |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by weifangsun@chromium.org
, Jul 30