NaClBrowser implementation has several threading issues |
||||
Issue descriptionNaClBrowser runs mostly on the browser IO thread, but is accessed in unsafe ways from several other threads: 1. NaClBrowserDelegate is set from the UI thread, but used by NaClBrowser on the IO thread. This looks OK in principle, since NaClBrowser owns the delegate, except that the Delegate interface doesn't specify any threading nor lifetime expectations, and is used from various call-sites from various threads. 2. NaClBrowser runs primarily in the IO thread but is a Singleton, so torn down on the UI thread. 3. NaClBrowser uses WeakPtr to dispatch file IO to the FILE thread and Bind() itself to receive replies. This doesn't actually work as intended since, as per (2), NaClBrowser is a Singleton, so the WeakPtrs are dereferenced on the IO thread but invalidated on the UI thread during deletion, which doesn't work. It's questionable whether WeakPtr is useful at all, since the IO and FILE threads will have been torn down before the Singleton is deleted. 4. NaClBrowser is a Singleton, notionally torn down on process exit, but lives in the Browser process, which AFAICT does not use AtExitManager. 5. NaClBrowser holds the GDB debug port mapping, which is written on the IO thread but read by TaskGroup on the UI thread. This should only be an issue if Chrome is configured for NaCl debugging, and Task Manager happens to be open when you launch a NaCl process, but it risks crashing the browser.
,
Jan 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f0bd838cce8870a95aec33be0b58151bd80f96aa commit f0bd838cce8870a95aec33be0b58151bd80f96aa Author: wez <wez@chromium.org> Date: Thu Jan 19 21:47:06 2017 Query NaCl processes' GDB debug port on the correct thread. TaskGroup previously called NaClBrowser on the UI thread, to refresh the GDB debug port information for NaCl processes. NaClBrowser is designed to be accessed only from the Browser's IO thread; this patch fixes that. BUG= 630861 Review-Url: https://codereview.chromium.org/2634573003 Cr-Commit-Position: refs/heads/master@{#444849} [modify] https://crrev.com/f0bd838cce8870a95aec33be0b58151bd80f96aa/chrome/browser/task_manager/sampling/task_group.cc [modify] https://crrev.com/f0bd838cce8870a95aec33be0b58151bd80f96aa/chrome/browser/task_manager/sampling/task_group.h
,
Jan 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/01f28a09b779a3081b4c485dae8e25e7fe8eba40 commit 01f28a09b779a3081b4c485dae8e25e7fe8eba40 Author: wez <wez@chromium.org> Date: Mon Jan 23 21:12:34 2017 Revert of Query NaCl processes' GDB debug port on the correct thread. (patchset #1 id:1 of https://codereview.chromium.org/2634573003/ ) Reason for revert: This fix introduced a typo in the background refresh flags mask, breaking it. Original issue's description: > Query NaCl processes' GDB debug port on the correct thread. > > TaskGroup previously called NaClBrowser on the UI thread, to refresh the > GDB debug port information for NaCl processes. NaClBrowser is designed > to be accessed only from the Browser's IO thread; this patch fixes that. > > BUG= 630861 > > Review-Url: https://codereview.chromium.org/2634573003 > Cr-Commit-Position: refs/heads/master@{#444849} > Committed: https://chromium.googlesource.com/chromium/src/+/f0bd838cce8870a95aec33be0b58151bd80f96aa TBR=afakhry@chromium.org,bradnelson@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 630861 , 684042 Review-Url: https://codereview.chromium.org/2649063003 Cr-Commit-Position: refs/heads/master@{#445485} [modify] https://crrev.com/01f28a09b779a3081b4c485dae8e25e7fe8eba40/chrome/browser/task_manager/sampling/task_group.cc [modify] https://crrev.com/01f28a09b779a3081b4c485dae8e25e7fe8eba40/chrome/browser/task_manager/sampling/task_group.h
,
Jan 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/621ea63f81c4620ce4455c05cdbbffd4e0aa1af6 commit 621ea63f81c4620ce4455c05cdbbffd4e0aa1af6 Author: wez <wez@chromium.org> Date: Wed Jan 25 18:10:02 2017 Revert of Query NaCl processes' GDB debug port on the correct thread. (patchset #1 id:1 of https://codereview.chromium.org/2634573003/ ) Reason for revert: This fix introduced a typo in the background refresh flags mask, breaking it. Original issue's description: > Query NaCl processes' GDB debug port on the correct thread. > > TaskGroup previously called NaClBrowser on the UI thread, to refresh the > GDB debug port information for NaCl processes. NaClBrowser is designed > to be accessed only from the Browser's IO thread; this patch fixes that. > > BUG= 630861 > > Review-Url: https://codereview.chromium.org/2634573003 > Cr-Commit-Position: refs/heads/master@{#444849} > Committed: https://chromium.googlesource.com/chromium/src/+/f0bd838cce8870a95aec33be0b58151bd80f96aa NOTRY=true NOPRESUBMIT=true TBR=afakhry@chromium.org,bradnelson@chromium.org BUG= 630861 , 684042 Review-Url: https://codereview.chromium.org/2649063003 Cr-Commit-Position: refs/heads/master@{#445485} (cherry picked from commit 01f28a09b779a3081b4c485dae8e25e7fe8eba40) Review-Url: https://codereview.chromium.org/2652083003 Cr-Commit-Position: refs/branch-heads/2987@{#89} Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943} [modify] https://crrev.com/621ea63f81c4620ce4455c05cdbbffd4e0aa1af6/chrome/browser/task_manager/sampling/task_group.cc [modify] https://crrev.com/621ea63f81c4620ce4455c05cdbbffd4e0aa1af6/chrome/browser/task_manager/sampling/task_group.h
,
Jan 26 2017
,
Feb 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/91503efd94b63d9be85603e7d1d6edf7e837f4c9 commit 91503efd94b63d9be85603e7d1d6edf7e837f4c9 Author: wez <wez@chromium.org> Date: Fri Feb 17 02:26:43 2017 Query NaCl processes' GDB debug port on the correct thread. TaskGroup previously called NaClBrowser on the UI thread, to refresh the GDB debug port information for NaCl processes. NaClBrowser is designed to be accessed only from the Browser's IO thread; this patch fixes that.Fix NaCl debug stub port fetching in Task Manager. This is a re-land of crrev.com/2634573003 which had a couple of issues. BUG= 630861 , 684042 Review-Url: https://codereview.chromium.org/2646623004 Cr-Commit-Position: refs/heads/master@{#451188} [modify] https://crrev.com/91503efd94b63d9be85603e7d1d6edf7e837f4c9/chrome/browser/task_manager/sampling/task_group.cc [modify] https://crrev.com/91503efd94b63d9be85603e7d1d6edf7e837f4c9/chrome/browser/task_manager/sampling/task_group.h [add] https://crrev.com/91503efd94b63d9be85603e7d1d6edf7e837f4c9/chrome/browser/task_manager/sampling/task_group_unittest.cc [modify] https://crrev.com/91503efd94b63d9be85603e7d1d6edf7e837f4c9/chrome/browser/ui/task_manager/task_manager_table_model.cc [modify] https://crrev.com/91503efd94b63d9be85603e7d1d6edf7e837f4c9/chrome/test/BUILD.gn
,
Feb 24 2017
Landed https://codereview.chromium.org/2630443003/: Add thread checks to NaClBrowser, and make it leaky. This makes the following fixes to NaClBrowser: 1. Removes use of WeakPtr to allow in-progress file I/O to be abandoned when NaClBrowser is torn-down; this never worked as intended, since the WeakPtrs were used on the I/O thread while NaClBrowser was torn- down via the Singleton::OnExit handler, on the UI thread. 2. Adds thread checks to NaClBrowser to ensure it is only called from the browser's I/O thread. 3. Changes the signature of SetDelegate() to express that the caller no longer owns the supplied NaClBrowserDelegate. 4. Leaks the NaClBrowser and NaClBrowserDelegate, rather than cleaning them up on exit, since at present they have no cleanup to do. Review-Url: https://codereview.chromium.org/2630443003 Cr-Commit-Position: refs/heads/master@{#452697} Committed: https://chromium.googlesource.com/chromium/src/+/72c5d6e04f39f920ea41eb20d56c593fbf83a70c |
||||
►
Sign in to add a comment |
||||
Comment 1 by w...@chromium.org
, Jan 13 2017Labels: M-57
Owner: w...@chromium.org
Status: Started (was: Untriaged)