Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2601733002:
IntersectionObserverCallbackImpl should use Member<ExecutionContext> (Closed)
DescriptionIntersectionObserverCallbackImpl should use Member<ExecutionContext>
Currently IntersectionObserverCallbackImpl uses WeakMember<ExecutionContext>
but I don't see any reason it has to be a weak member.
Also some call site assumes that IntersectionObserverCallbackImpl::getExecutionContext()
doesn't return null.
So it looks better to let IntersectionObserverCallbackImpl own Member<ExecutionContext>.
BUG=610176
Committed: https://crrev.com/61862d1ea4b844b5b5051ea34945284b4a5dac71
Cr-Commit-Position: refs/heads/master@{#440714}
Patch Set 1 #
Total comments: 1
Messages
Total messages: 18 (8 generated)
haraken@chromium.org changed reviewers: + sigbjornf@opera.com
PTAL
lgtm to try, watch for leaks. With the preference for using closures to bind & hold onto some of the intersection observer objects, implicit Persistent<>s can make it harder to reason about leaking. https://codereview.chromium.org/2601733002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right): https://codereview.chromium.org/2601733002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:57: Member<ExecutionContext> m_context; The use of WeakMember<> comes from https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Sou... not much discussion associated with it. If there are problems here with keeping a strong reference, they'd be leaks.
The CQ bit was checked by haraken@chromium.org
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by haraken@chromium.org
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": 1, "attempt_start_ts": 1482750802884250, "parent_rev":
"cc0d2950835a627a7caccf2c6cb624660f4415a9", "commit_rev":
"f101a1e5310c2f9639079704ecd89c1b60f75849"}
Message was sent while issue was closed.
Description was changed from ========== IntersectionObserverCallbackImpl should use Member<ExecutionContext> Currently IntersectionObserverCallbackImpl uses WeakMember<ExecutionContext> but I don't see any reason it has to be a weak member. Also some call site assumes that IntersectionObserverCallbackImpl::getExecutionContext() doesn't return null. So it looks better to let IntersectionObserverCallbackImpl own Member<ExecutionContext>. BUG=610176 ========== to ========== IntersectionObserverCallbackImpl should use Member<ExecutionContext> Currently IntersectionObserverCallbackImpl uses WeakMember<ExecutionContext> but I don't see any reason it has to be a weak member. Also some call site assumes that IntersectionObserverCallbackImpl::getExecutionContext() doesn't return null. So it looks better to let IntersectionObserverCallbackImpl own Member<ExecutionContext>. BUG=610176 Review-Url: https://codereview.chromium.org/2601733002 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== IntersectionObserverCallbackImpl should use Member<ExecutionContext> Currently IntersectionObserverCallbackImpl uses WeakMember<ExecutionContext> but I don't see any reason it has to be a weak member. Also some call site assumes that IntersectionObserverCallbackImpl::getExecutionContext() doesn't return null. So it looks better to let IntersectionObserverCallbackImpl own Member<ExecutionContext>. BUG=610176 Review-Url: https://codereview.chromium.org/2601733002 ========== to ========== IntersectionObserverCallbackImpl should use Member<ExecutionContext> Currently IntersectionObserverCallbackImpl uses WeakMember<ExecutionContext> but I don't see any reason it has to be a weak member. Also some call site assumes that IntersectionObserverCallbackImpl::getExecutionContext() doesn't return null. So it looks better to let IntersectionObserverCallbackImpl own Member<ExecutionContext>. BUG=610176 Committed: https://crrev.com/61862d1ea4b844b5b5051ea34945284b4a5dac71 Cr-Commit-Position: refs/heads/master@{#440714} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/61862d1ea4b844b5b5051ea34945284b4a5dac71 Cr-Commit-Position: refs/heads/master@{#440714}
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2600143002/ by avi@chromium.org. The reason for reverting is: Crashes telemetry bots. https://build.chromium.org/p/chromium.mac/builders/Mac10.10%20Tests/builds/11254 https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%281%29/bui... 005be100 67b85543 00000012 212a2238 00000000 chrome_child!blink::IntersectionObserver::computeIntersectionObservations+0x2e 005be140 67c44f75 212a2238 005be184 67c43f05 chrome_child!blink::IntersectionObserverController::computeTrackedIntersectionObservations+0xa2 005be14c 67c43f05 00000012 6939fcdc 2b941820 chrome_child!blink::FrameView::updateViewportIntersectionsForSubtree+0x3b 005be184 67c439fd 00000012 67e3e961 384b0000 chrome_child!blink::FrameView::updateLifecyclePhasesInternal+0x22f 005be18c 67e3e961 384b0000 00000000 005be1a8 chrome_child!blink::FrameView::updateAllLifecyclePhases+0x18 005be19c 679b9ce1 318e1a88 005be1e0 679a77e5 chrome_child!blink::PageAnimator::updateAllLifecyclePhases+0x1c 005be1a8 679a77e5 318e1820 318e1a88 0139c080 chrome_child!blink::PageWidgetDelegate::updateAllLifecyclePhases+0x11 005be1e0 684c45d9 049c90fc 04952dd0 00000000 chrome_child!blink::WebViewImpl::updateAllLifecyclePhases+0xa2 005be354 684cd388 04952dd0 00000000 0165ca70 chrome_child!cc::ProxyMain::BeginMainFrame+0x409 005be4d0 684ceba6 049c90f8 684c41d0 049c90e8 chrome_child!base::internal::Invoker<base::internal::BindState<void (__thiscall cc::ProxyMain::*)(std::unique_ptr<cc::BeginMainFrameAndCommitState,std::default_delete<cc::BeginMainFrameAndCommitState> >),base::WeakPtr<cc::ProxyMain>,base::internal::PassedWrapper<std::unique_ptr<cc::BeginMainFrameAndCommitState,std::default_delete<cc::BeginMainFrameAndCommitState> > > >,void __cdecl(void)>::RunImpl<void (__thiscall cc::ProxyMain::*const &)(std::unique_ptr<cc::BeginMainFrameAndCommitState,std::default_delete<cc::BeginMainFrameAndCommitState> >),std::tuple<base::WeakPtr<cc::ProxyMain>,base::internal::PassedWrapper<std::unique_ptr<cc::BeginMainFrameAndCommitState,std::default_delete<cc::BeginMainFrameAndCommitState> > > > const &,0,1>+0x98 005be4e4 671ff214 049c90e8 0165dcd0 00000000 chrome_child!base::internal::Invoker<base::internal::BindState<void (__thiscall cc::ProxyMain::*)(std::unique_ptr<cc::BeginMainFrameAndCommitState,std::default_delete<cc::BeginMainFrameAndCommitState> >),base::WeakPtr<cc::ProxyMain>,base::internal::PassedWrapper<std::unique_ptr<cc::BeginMainFrameAndCommitState,std::default_delete<cc::BeginMainFrameAndCommitState> > > >,void __cdecl(void)>::Run+0x16 005be5a4 67962b56 68e02cb4 005be8e0 005be9dc chrome_child!base::debug::TaskAnnotator::RunTask+0x164 005be9cc 679622cc 0165be08 005bea0c 0165c958 chrome_child!blink::scheduler::TaskQueueManager::ProcessTaskFromWorkQueue+0x3dc 005bea38 6796176f 00000000 00000000 00000003 chrome_child!blink::scheduler::TaskQueueManager::DoWork+0x119 005bea4c 679617b5 679621b3 00000000 0165bc38 chrome_child!base::internal::FunctorTraits<void (__thiscall blink::scheduler::TaskQueueManager::*)(base::TimeTicks,bool),void>::Invoke<base::WeakPtr<blink::scheduler::TaskQueueManager> const &,base::TimeTicks const &,bool const &>+0x22 005bea68 679617d1 0165bc20 0165bc38 0165bc30 chrome_child!base::internal::InvokeHelper<1,void>::MakeItSo<void (__thiscall blink::scheduler::TaskQueueManager::*const &)(base::TimeTicks,bool),base::WeakPtr<blink::scheduler::TaskQueueManager> const &,base::TimeTicks const &,bool const &>+0x25 005bea80 67962e9d 0165bc20 0165bc28 0165bc10 chrome_child!base::internal::Invoker<base::internal::BindState<void (__thiscall blink::scheduler::TaskQueueManager::*)(base::TimeTicks,bool),base::WeakPtr<blink::scheduler::TaskQueueManager>,base::TimeTicks,bool>,void __cdecl(void)>::RunImpl<void (__thiscall blink::scheduler::TaskQueueManager::*const &)(base::TimeTicks,bool),std::tuple<base::WeakPtr<blink::scheduler::TaskQueueManager>,base::TimeTicks,bool> const &,0,1,2>+0x17 005bea94 671ff214 0165bc10 00000000 00000000 chrome_child!base::internal::Invoker<base::internal::BindState<void (__thiscall blink::scheduler::TaskQueueManager::*)(base::TimeTicks,bool),base::WeakPtr<blink::scheduler::TaskQueueManager>,base::TimeTicks,bool>,void __cdecl(void)>::Run+0x16 005beb54 671cb5ad 68bfd37c 005bf830 0165c958 chrome_child!base::debug::TaskAnnotator::RunTask+0x164 005bf7b8 671caa75 005bf830 0164adb8 0165c958 chrome_child!base::MessageLoop::RunTask+0x4cd 005bf874 67200714 00000000 0165c958 00000000 chrome_child!base::MessageLoop::DoWork+0x255 005bf88c 671cb0ce 0165c958 005bf9dc 68b17d6c chrome_child!base::MessagePumpDefault::Run+0x74 005bf954 671d95af 68f11b10 00000000 687c6210 chrome_child!base::MessageLoop::RunHandler+0x5e 005bf9c8 6816baa8 01631980 00000003 01632a28 chrome_child!base::RunLoop::Run+0x6f 005bfaa4 67186ea4 005bfadc 0163d818 00000000 chrome_child!content::RendererMain+0x1e6 005bfab8 67186e12 005bfaf4 005bfadc 005bfbe4 chrome_child!content::RunNamedProcessTypeMain+0x61 005bfb10 671864d9 0100b768 6d004270 005bfc18 chrome_child!content::ContentMainRunnerImpl::Run+0x97 005bfb20 668fd5a7 005bfc00 668fd4f0 0100b770 chrome_child!content::ContentMain+0x23 .
Message was sent while issue was closed.
szager@chromium.org changed reviewers: + szager@chromium.org
Message was sent while issue was closed.
In the future, please add me to IntersectionObserver changes. The object lifetime management of IntersectionObserver is complicated, and it would be easy to introduce reference leaks. It is important that an IntersectionObserverCallbackImpl not keep its ExecutionContext alive. There is one un-guarded access to getExecutionContext(), but at that point in the code, a DCHECK(getExecutionContext()) would be appropriate.
Message was sent while issue was closed.
On 2017/01/04 22:24:32, szager1 wrote: > In the future, please add me to IntersectionObserver changes. Ah, snap; I see that I am a reviewer; sorry for the terse reply. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
