Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1901023003:
Introduce browser side FrameTreeNodeBlameContext (Closed)
|
Created:
4 years, 8 months ago by Xiaocheng Modified:
4 years, 7 months ago CC:
chromium-reviews, creis+watch_chromium.org, Charlie Reis, darin-cc_chromium.org, jam, nasko+codewatch_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce browser side FrameTreeNodeBlameContext
This patch introduces a new class |FrameTreeNodeBlameContext|, which is a
subclass of |BlameContext|, that maintains the snapshots of browser side
frame tree nodes, and enables automatic dumping of the frame tree at
browser side when tracing starts.
This patch also removes the |TracedFrameTreeNode| class because its
functionality is subsumed by |FrameTreeNodeBlameContext|. Some of the logic
of the former class is moved into the latter.
BUG=546021
TEST=content_unittests --gtest_filter=FrameTreeNodeBlameContextTest.*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Patch Set 1 #
Total comments: 11
Patch Set 2 : Revised the comments slightly #
Total comments: 7
Patch Set 3 : Add locking and documentation #
Total comments: 2
Patch Set 4 : Fix locking and add unit test #Patch Set 5 : Rebase #Patch Set 6 : Fix compile error #
Total comments: 1
Patch Set 7 : Remove thread safety handling (and its doc) #Depends on Patchset: Messages
Total messages: 35 (9 generated)
Description was changed from ========== Enable browser side frame tree dumping at tracing startup This patch introduces a new class FrameTreeNodeBlameContext, which is a subclass of BlameContext, that maintains the snapshots of browser side frame tree nodes, and enables automatic dumping of the frame tree at browser side when tracing starts. BUG=546021 TEST=still working... ========== to ========== Enable browser side frame tree dumping at tracing startup This patch introduces a new class FrameTreeNodeBlameContext, which is a subclass of BlameContext, that maintains the snapshots of browser side frame tree nodes, and enables automatic dumping of the frame tree at browser side when tracing starts. BUG=546021 TEST=still working... CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Enable browser side frame tree dumping at tracing startup This patch introduces a new class FrameTreeNodeBlameContext, which is a subclass of BlameContext, that maintains the snapshots of browser side frame tree nodes, and enables automatic dumping of the frame tree at browser side when tracing starts. BUG=546021 TEST=still working... CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Enable browser side frame tree dumping at tracing startup This patch introduces a new class |FrameTreeNodeBlameContext|, which is a subclass of |BlameContext|, that maintains the snapshots of browser side frame tree nodes, and enables automatic dumping of the frame tree at browser side when tracing starts. This patch also removes the |TracedFrameTreeNode| class because its functionality is subsumed by |FrameTreeNodeBlameContext|. Some of the logic of the former class is moved into the latter. BUG=546021 TEST=still working... CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Enable browser side frame tree dumping at tracing startup This patch introduces a new class |FrameTreeNodeBlameContext|, which is a subclass of |BlameContext|, that maintains the snapshots of browser side frame tree nodes, and enables automatic dumping of the frame tree at browser side when tracing starts. This patch also removes the |TracedFrameTreeNode| class because its functionality is subsumed by |FrameTreeNodeBlameContext|. Some of the logic of the former class is moved into the latter. BUG=546021 TEST=still working... CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Introduce browser side FrameTreeNodeBlameContext This patch introduces a new class |FrameTreeNodeBlameContext|, which is a subclass of |BlameContext|, that maintains the snapshots of browser side frame tree nodes, and enables automatic dumping of the frame tree at browser side when tracing starts. This patch also removes the |TracedFrameTreeNode| class because its functionality is subsumed by |FrameTreeNodeBlameContext|. Some of the logic of the former class is moved into the latter. BUG=546021 TEST=still working... CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
benjhayden@chromium.org changed reviewers: + benjhayden@chromium.org
Thanks! This looks like a neat idea that might simplify some things and enable some new features. Do you want to work with skyostil to see where we might be able to call Enter/Leave on this new blame context so that we can attribute tasks in the browser to frames? https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node.cc:492: blame_context_ = new FrameTreeNodeBlameContext(this); Where is this object deleted? https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node.h:353: FrameTreeNodeBlameContext* blame_context_; // Not owned. FrameTreeNode creates it. Why doesn't it own it? https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node_blame_context.cc (right): https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node_blame_context.cc:62: void FrameTreeNodeBlameContext::AsValueInto( Hm, it looks like there might be a gotcha here: this method may be called on either the UI or the FILE thread, depending on the caller. OnTraceLogEnabled/Disabled can be called on the FILE thread, iirc. As it is, this doesn't look like a problem, but it definitely smells odd to me. Most of the rest of chrome can usually rely on being called on one particular thread. FrameTree and friends all live on the UI thread, so you can rely on the invariant that their methods are always called on the UI thread and their members are only accessed on the UI thread. Chrome devs are accustomed to working under that single-thread invariant per-object or at least per-method. Without it, data races and other threading bugs can creep in. It's definitely neat how UpdateArguments lets you buffer the args in a long-lived object so you don't need to worry about it. But it might be too neat if it lets you play fast and loose with threads. Now that I look through some of the StartTracing callers, it looks like some of them might be on the UI thread, but not all. So perhaps the gotcha is there. Maybe BlameContext should use a WeakPtrFactory to guarantee that AsValueInto is always called on a specified thread? Or maybe YAGNI? Sorry I'm not more helpful. nduca is on vacation, so maybe skyostil can help?
Thanks for the review (which is fast)! https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node.cc:148: child->InitializeBlameContext(); One more question: BlameContext::parent_id_ is declared as a const, but the topology of the frame tree can be changed -- we can, say, create a new iframe and then put all the other existing iframes into it. Does it introduce any issue, e.g., reassigning the parent of an existing BlameContext? https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node.cc:492: blame_context_ = new FrameTreeNodeBlameContext(this); On 2016/04/20 at 01:37:34, benjhayden_chromium wrote: > Where is this object deleted? See the reply for the last comment. https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node.h:353: FrameTreeNodeBlameContext* blame_context_; // Not owned. On 2016/04/20 at 01:37:34, benjhayden_chromium wrote: > FrameTreeNode creates it. Why doesn't it own it? I'm actually having the same question on the relation between RenderFrameImpl and FrameBlameContext. I studied the lifecycle of FrameBlameContext, but didn't find (or maybe just missed) any place where a FrameBlameContext gets deleted. Is there any substantial difference between (i) the relation between RenderFrameImpl and FrameBlameContext, and (ii) the relation between FrameTreeNode and FrameTreeNodeBlameContext? https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node_blame_context.cc (right): https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node_blame_context.cc:62: void FrameTreeNodeBlameContext::AsValueInto( On 2016/04/20 at 01:37:34, benjhayden_chromium wrote: > Hm, it looks like there might be a gotcha here: this method may be called on either the UI or the FILE thread, depending on the caller. OnTraceLogEnabled/Disabled can be called on the FILE thread, iirc. > > As it is, this doesn't look like a problem, but it definitely smells odd to me. Most of the rest of chrome can usually rely on being called on one particular thread. FrameTree and friends all live on the UI thread, so you can rely on the invariant that their methods are always called on the UI thread and their members are only accessed on the UI thread. Chrome devs are accustomed to working under that single-thread invariant per-object or at least per-method. Without it, data races and other threading bugs can creep in. > > It's definitely neat how UpdateArguments lets you buffer the args in a long-lived object so you don't need to worry about it. But it might be too neat if it lets you play fast and loose with threads. > > Now that I look through some of the StartTracing callers, it looks like some of them might be on the UI thread, but not all. So perhaps the gotcha is there. Maybe BlameContext should use a WeakPtrFactory to guarantee that AsValueInto is always called on a specified thread? Or maybe YAGNI? > > Sorry I'm not more helpful. nduca is on vacation, so maybe skyostil can help? Thanks for pointing out the gotcha and let me think about it...
benjhayden@chromium.org changed reviewers: + skyostil@chromium.org
Sami, where is the FrameBlameContext deleted?
A note for something I just found: FrameBlameContext::TakeSnapshot() is also called on multiple threads -- both CrRendererMain and Chrome_ChildIOThread. It seems OK, though, as FrameBlameContext is just a bunch of constants, and it doesn't access the RenderFrameImpl after initialized.
I have thought about the threading issue of BlameContext. Currently there are two types of dumps: 1. Dump at tracing startup, initiated from FileThread (for browser) or ChildIOThread (for renderer). 2. Dump when a main thread object (RenderFrameImpl, FrameTreeNode, and likely to grow when the project gets developed) deems necessary, initiated from the CrBrowserMain or CrRendererMain. Examples include the initiation of a BlameContext and the snapshots of FrameTreeNodes taken after navigation. If we stick to the principle that each object should be accessed from only one thread, then we either post type 1 dumps as main thread tasks, or post type 2 dumps as File/IO thread tasks. I think the better choice is to post type 1 dumps as main thread tasks with the following consideration: - Messaging is expensive, so we should make sure most dumps are handled without task posting - Type 1 dumps occur only at tracing startup, which seems to be much more infrequently than type 2 WDYT?
Now I understand what "a can of architectural worms" (quoted from https://codereview.chromium.org/1775953003#msg10) means. Fixing the threading issue seems an overly complicated and time-consuming task that I don't want to touch right now, as it requires a good understanding of all subclasses of StateEnabledObserver. In the current design (Patch 2), there is a race condition but can only happen in the following scenario: tracing starts and File thread calls AsValueInto while the UI thread is running UpdateArguments(), making AsValueInto generate undefined output. It seems to me that the undefined output is tolerable, considering that the mess it causes looks quite limited and under control but the effort required to fix it seems huge. We may treat it as a known caveat to be fixed in the future. For now, I think the most important task is to fix the lifecycle of BlameContext (and its subclasses): what objects own them and when they should be deleted.
On 2016/04/20 02:16:17, benjhayden_chromium wrote: > Sami, where is the FrameBlameContext deleted? Sorry, this was just a bug. The RenderFrame should own the context and I believe the same applies here. Fix: https://codereview.chromium.org/1907453002/
Thanks for working on this patch! https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node.cc:148: child->InitializeBlameContext(); On 2016/04/20 02:01:05, Xiaocheng wrote: > One more question: BlameContext::parent_id_ is declared as a const, but the > topology of the frame tree can be changed -- we can, say, create a new iframe > and then put all the other existing iframes into it. Does it introduce any > issue, e.g., reassigning the parent of an existing BlameContext? If the topology changes, the easiest thing to do is to create another blame context with the same id as before. If this is a problem, we can modify base::BlameContext to support changing the parent. https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node_blame_context.cc (right): https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node_blame_context.cc:62: void FrameTreeNodeBlameContext::AsValueInto( On 2016/04/20 02:01:05, Xiaocheng wrote: > On 2016/04/20 at 01:37:34, benjhayden_chromium wrote: > > Hm, it looks like there might be a gotcha here: this method may be called on > either the UI or the FILE thread, depending on the caller. > OnTraceLogEnabled/Disabled can be called on the FILE thread, iirc. > > > > As it is, this doesn't look like a problem, but it definitely smells odd to > me. Most of the rest of chrome can usually rely on being called on one > particular thread. FrameTree and friends all live on the UI thread, so you can > rely on the invariant that their methods are always called on the UI thread and > their members are only accessed on the UI thread. Chrome devs are accustomed to > working under that single-thread invariant per-object or at least per-method. > Without it, data races and other threading bugs can creep in. > > > > It's definitely neat how UpdateArguments lets you buffer the args in a > long-lived object so you don't need to worry about it. But it might be too neat > if it lets you play fast and loose with threads. > > > > Now that I look through some of the StartTracing callers, it looks like some > of them might be on the UI thread, but not all. So perhaps the gotcha is there. > Maybe BlameContext should use a WeakPtrFactory to guarantee that AsValueInto is > always called on a specified thread? Or maybe YAGNI? > > > > Sorry I'm not more helpful. nduca is on vacation, so maybe skyostil can help? > > Thanks for pointing out the gotcha and let me think about it... IMO the most straightforward way to avoid threading issues here is to save copies of all necessary fields on the right thread so they can be snapshotted on an arbitrary thread. Frames are created and modified rarely (relatively speaking) so this should not be any measurable performance overhead. You might still need a lock if the snapshot values need to change (e.g., url). Another option in that case would be to create a fresh BlameContext but that might complicate things if we want to Enter/Leave these contexts in the future.
creis@chromium.org changed reviewers: + creis@chromium.org
Drive-by: This class is surprising to me, since it doesn't explain much about how it will be used. Please try to clarify it in comments so that it's easy to see what it's for. (We've had similar problems with RendererFrameManager, which is unrelated to RenderFrameHostManager.) https://codereview.chromium.org/1901023003/diff/20001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/1901023003/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.h:269: FrameTreeNodeBlameContext* blame_context() const { return blame_context_; } These all need comments, because it's not obvious what "blame context" means in relation to the core FrameTreeNode logic. https://codereview.chromium.org/1901023003/diff/20001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node_blame_context.h (right): https://codereview.chromium.org/1901023003/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree_node_blame_context.h:21: class FrameTreeNodeBlameContext : public base::trace_event::BlameContext { This class needs comments explaining the context for this. (It could be misinterpreted as part of the core frame logic rather than tracing/scheduling.) https://codereview.chromium.org/1901023003/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree_node_blame_context.h:42: std::string url_; Why is a URL being stored in a string? I would recommend against that, but if it's necessary please explain why in a comment.
https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node_blame_context.cc (right): https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node_blame_context.cc:62: void FrameTreeNodeBlameContext::AsValueInto( On 2016/04/20 at 12:15:08, Sami wrote: > On 2016/04/20 02:01:05, Xiaocheng wrote: > > On 2016/04/20 at 01:37:34, benjhayden_chromium wrote: > > > Hm, it looks like there might be a gotcha here: this method may be called on > > either the UI or the FILE thread, depending on the caller. > > OnTraceLogEnabled/Disabled can be called on the FILE thread, iirc. > > > > > > As it is, this doesn't look like a problem, but it definitely smells odd to > > me. Most of the rest of chrome can usually rely on being called on one > > particular thread. FrameTree and friends all live on the UI thread, so you can > > rely on the invariant that their methods are always called on the UI thread and > > their members are only accessed on the UI thread. Chrome devs are accustomed to > > working under that single-thread invariant per-object or at least per-method. > > Without it, data races and other threading bugs can creep in. > > > > > > It's definitely neat how UpdateArguments lets you buffer the args in a > > long-lived object so you don't need to worry about it. But it might be too neat > > if it lets you play fast and loose with threads. > > > > > > Now that I look through some of the StartTracing callers, it looks like some > > of them might be on the UI thread, but not all. So perhaps the gotcha is there. > > Maybe BlameContext should use a WeakPtrFactory to guarantee that AsValueInto is > > always called on a specified thread? Or maybe YAGNI? > > > > > > Sorry I'm not more helpful. nduca is on vacation, so maybe skyostil can help? > > > > Thanks for pointing out the gotcha and let me think about it... > > IMO the most straightforward way to avoid threading issues here is to save copies of all necessary fields on the right thread so they can be snapshotted on an arbitrary thread. Frames are created and modified rarely (relatively speaking) so this should not be any measurable performance overhead. > > You might still need a lock if the snapshot values need to change (e.g., url). Another option in that case would be to create a fresh BlameContext but that might complicate things if we want to Enter/Leave these contexts in the future. If re-creating the BlameContext when the url changes can complicate using Enter/Leave (which seems like a logical way to attribute RenderFrame, etc. tasks to the frame, which would simplify finding LoadExpectations), then should BlameContext support changing the parent_id without re-creation? Is there a reason not to make TraceLog::EnabledStateObserver run on a single thread so that clients such as BlameContexts don't need to worry about locks and WeakPtrFactories? Maybe that's a job for TracingV2 or the fixit? https://codereview.chromium.org/1901023003/diff/20001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node_blame_context.h (right): https://codereview.chromium.org/1901023003/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree_node_blame_context.h:42: std::string url_; On 2016/04/20 at 16:58:58, Charlie Reis wrote: > Why is a URL being stored in a string? I would recommend against that, but if it's necessary please explain why in a comment. That's my fault. You're right, there should be a comment. This field will only ever be serialized to the JSON trace log. (Actually, the trace log format could change, but it probably won't ever use anything besides a string for URLs. Probably.) I wanted to be explicit about how the URL would be converted to a string, instead of burying it under layers like TracedValue and BlameContext. I'd be open to storing it as a GURL here and stringifying it in AsValueInto() if you think that would be better.
Done with thread safety and documentation. https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node_blame_context.cc (right): https://codereview.chromium.org/1901023003/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node_blame_context.cc:62: void FrameTreeNodeBlameContext::AsValueInto( On 2016/04/20 at 17:29:45, benjhayden_chromium wrote: > On 2016/04/20 at 12:15:08, Sami wrote: > > On 2016/04/20 02:01:05, Xiaocheng wrote: > > > On 2016/04/20 at 01:37:34, benjhayden_chromium wrote: > > > > Hm, it looks like there might be a gotcha here: this method may be called on > > > either the UI or the FILE thread, depending on the caller. > > > OnTraceLogEnabled/Disabled can be called on the FILE thread, iirc. > > > > > > > > As it is, this doesn't look like a problem, but it definitely smells odd to > > > me. Most of the rest of chrome can usually rely on being called on one > > > particular thread. FrameTree and friends all live on the UI thread, so you can > > > rely on the invariant that their methods are always called on the UI thread and > > > their members are only accessed on the UI thread. Chrome devs are accustomed to > > > working under that single-thread invariant per-object or at least per-method. > > > Without it, data races and other threading bugs can creep in. > > > > > > > > It's definitely neat how UpdateArguments lets you buffer the args in a > > > long-lived object so you don't need to worry about it. But it might be too neat > > > if it lets you play fast and loose with threads. > > > > > > > > Now that I look through some of the StartTracing callers, it looks like some > > > of them might be on the UI thread, but not all. So perhaps the gotcha is there. > > > Maybe BlameContext should use a WeakPtrFactory to guarantee that AsValueInto is > > > always called on a specified thread? Or maybe YAGNI? > > > > > > > > Sorry I'm not more helpful. nduca is on vacation, so maybe skyostil can help? > > > > > > Thanks for pointing out the gotcha and let me think about it... > > > > IMO the most straightforward way to avoid threading issues here is to save copies of all necessary fields on the right thread so they can be snapshotted on an arbitrary thread. Frames are created and modified rarely (relatively speaking) so this should not be any measurable performance overhead. > > > > You might still need a lock if the snapshot values need to change (e.g., url). Another option in that case would be to create a fresh BlameContext but that might complicate things if we want to Enter/Leave these contexts in the future. > > If re-creating the BlameContext when the url changes can complicate using Enter/Leave (which seems like a logical way to attribute RenderFrame, etc. tasks to the frame, which would simplify finding LoadExpectations), then should BlameContext support changing the parent_id without re-creation? > > Is there a reason not to make TraceLog::EnabledStateObserver run on a single thread so that clients such as BlameContexts don't need to worry about locks and WeakPtrFactories? Maybe that's a job for TracingV2 or the fixit? To me, adding a lock seems to be the most painless way to ensure thread safety, as it's easy to implement and does not introduce any architectural change. I agree that there shouldn't be any measurable performance change with locks since navigation is rare. I've added a documentation stating explicitly that thread safety is achieved by locking, so that anyone interested in eliminating blocking calls can find and fix it. https://codereview.chromium.org/1901023003/diff/20001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/1901023003/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.h:269: FrameTreeNodeBlameContext* blame_context() const { return blame_context_; } On 2016/04/20 at 16:58:58, Charlie Reis wrote: > These all need comments, because it's not obvious what "blame context" means in relation to the core FrameTreeNode logic. Done. https://codereview.chromium.org/1901023003/diff/20001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node_blame_context.h (right): https://codereview.chromium.org/1901023003/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree_node_blame_context.h:21: class FrameTreeNodeBlameContext : public base::trace_event::BlameContext { On 2016/04/20 at 16:58:58, Charlie Reis wrote: > This class needs comments explaining the context for this. (It could be misinterpreted as part of the core frame logic rather than tracing/scheduling.) Done. https://codereview.chromium.org/1901023003/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree_node_blame_context.h:42: std::string url_; On 2016/04/20 at 17:29:45, benjhayden_chromium wrote: > On 2016/04/20 at 16:58:58, Charlie Reis wrote: > > Why is a URL being stored in a string? I would recommend against that, but if it's necessary please explain why in a comment. > > That's my fault. You're right, there should be a comment. > This field will only ever be serialized to the JSON trace log. > (Actually, the trace log format could change, but it probably won't ever use anything besides a string for URLs. Probably.) > I wanted to be explicit about how the URL would be converted to a string, instead of burying it under layers like TracedValue and BlameContext. > I'd be open to storing it as a GURL here and stringifying it in AsValueInto() if you think that would be better. I agree that a URL shouldn't be stored as a string. Fixed.
I'm still working on designing some FrameTreeNodeBlameContextTest. Due to my limited experience, the first issue that came into my mind is where the tests should go: content_browsertests, content_unittests or something else (which I don't know yet because I have enumerated everything I know here...)? I noticed that FrameTreeTest is in content_unittests. The FrameTreeNodeBlameContextTest should share a lot of common logic with FrameTreeTest, since both of them involves a lot of frame tree manipulation. However, FrameTreeNodeBlameContextTest also requires tracing, so I'm not sure if things can be done within content_unittests. In addition, how do I examine the trace output? Do I follow the design of BlameContextTest? All of the above are just gut feelings and might be completely wrong. Any advice is appreciated.
https://codereview.chromium.org/1901023003/diff/40001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node_blame_context.cc (right): https://codereview.chromium.org/1901023003/diff/40001/content/browser/frame_h... content/browser/frame_host/frame_tree_node_blame_context.cc:35: process_id_ = -1; lock?
creis@chromium.org changed reviewers: - creis@chromium.org
Thanks for the docs! That resolves my concern. Moving myself to CC, unless you need me further.
https://codereview.chromium.org/1901023003/diff/40001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node_blame_context.cc (right): https://codereview.chromium.org/1901023003/diff/40001/content/browser/frame_h... content/browser/frame_host/frame_tree_node_blame_context.cc:35: process_id_ = -1; On 2016/04/22 at 20:34:15, benjhayden_chromium wrote: > lock? You're right, this part should also be protected by the lock.
Description was changed from ========== Introduce browser side FrameTreeNodeBlameContext This patch introduces a new class |FrameTreeNodeBlameContext|, which is a subclass of |BlameContext|, that maintains the snapshots of browser side frame tree nodes, and enables automatic dumping of the frame tree at browser side when tracing starts. This patch also removes the |TracedFrameTreeNode| class because its functionality is subsumed by |FrameTreeNodeBlameContext|. Some of the logic of the former class is moved into the latter. BUG=546021 TEST=still working... CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Introduce browser side FrameTreeNodeBlameContext This patch introduces a new class |FrameTreeNodeBlameContext|, which is a subclass of |BlameContext|, that maintains the snapshots of browser side frame tree nodes, and enables automatic dumping of the frame tree at browser side when tracing starts. This patch also removes the |TracedFrameTreeNode| class because its functionality is subsumed by |FrameTreeNodeBlameContext|. Some of the logic of the former class is moved into the latter. BUG=546021 TEST=content_unittests --gtest_filter=FrameTreeNodeBlameContextTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
xiaochengh@chromium.org changed reviewers: + clamy@chromium.org
PTAL at Patch 4, which seems landable to me.
Sorry, PTAL at Patch 6.
Thanks for the patch. One way that we could make this simpler would be to make BlameContext have an idea of the owning thread for the object. Basically it could grab ThreadTaskRunnerHandle::Get() in the constructor and make sure TakeSnapshot() always runs on that thread. That would let us avoid the nasty locking and process handle lifetime issues here. WDYT?
On 2016/04/25 at 19:28:50, skyostil wrote: > Thanks for the patch. One way that we could make this simpler would be to make BlameContext have an idea of the owning thread for the object. Basically it could grab ThreadTaskRunnerHandle::Get() in the constructor and make sure TakeSnapshot() always runs on that thread. That would let us avoid the nasty locking and process handle lifetime issues here. WDYT? That SGTM.
lgtm
On 2016/04/25 at 19:28:50, skyostil wrote: > Thanks for the patch. One way that we could make this simpler would be to make BlameContext have an idea of the owning thread for the object. Basically it could grab ThreadTaskRunnerHandle::Get() in the constructor and make sure TakeSnapshot() always runs on that thread. That would let us avoid the nasty locking and process handle lifetime issues here. WDYT? SGTM too. I'll do it in a separate patch.
I should have noticed the revert of making RenderFrameImpl owning FrameBlameContext earlier: https://codereview.chromium.org/1909403002. Since this patch is following a similar pattern, it's likely to cause a similar use-after-free if landed. Maybe we should just let FrameTreeNode store a raw pointer to its blame context and let the memory leak for now, and later fix the leak as a separate bug?
On 2016/04/26 07:58:02, Xiaocheng wrote: > I should have noticed the revert of making RenderFrameImpl owning > FrameBlameContext earlier: https://codereview.chromium.org/1909403002. Since > this patch is following a similar pattern, it's likely to cause a similar > use-after-free if landed. > > Maybe we should just let FrameTreeNode store a raw pointer to its blame context > and let the memory leak for now, and later fix the leak as a separate bug? I believe the use after free there was caused by the scheduler keeping a pointer to a blame context after the frame went away. I still need to track down exactly what is going on but I believe it should be solvable. Since we don't (yet) have a task scheduler in the browser process, I don't think there is any lifetime issue here.
On 2016/04/26 at 10:33:18, skyostil wrote: > On 2016/04/26 07:58:02, Xiaocheng wrote: > > I should have noticed the revert of making RenderFrameImpl owning > > FrameBlameContext earlier: https://codereview.chromium.org/1909403002. Since > > this patch is following a similar pattern, it's likely to cause a similar > > use-after-free if landed. > > > > Maybe we should just let FrameTreeNode store a raw pointer to its blame context > > and let the memory leak for now, and later fix the leak as a separate bug? > > I believe the use after free there was caused by the scheduler keeping a pointer to a blame context after the frame went away. I still need to track down exactly what is going on but I believe it should be solvable. Since we don't (yet) have a task scheduler in the browser process, I don't think there is any lifetime issue here. I see. So there is no need to modify the lifecycle of FrameTreeNodeBlameContext in this patch.
Drive-by. https://codereview.chromium.org/1901023003/diff/100001/content/browser/frame_... File content/browser/frame_host/frame_tree_node_blame_context.h (right): https://codereview.chromium.org/1901023003/diff/100001/content/browser/frame_... content/browser/frame_host/frame_tree_node_blame_context.h:58: FrameTreeNodeBlameContext(FrameTreeNode* node); Nit: explicit
Closing this CL because it is stale after the landing of AsyncEnabledStateObserver. It will be replaced by a new CL for the same purpose. |
