Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1862073002:
Let AssociatedURLLoader listen to destruction of the Document used for loading (Closed)
|
Created:
4 years, 8 months ago by tyoshino (SeeGerritForStatus) Modified:
4 years, 8 months ago CC:
chromium-reviews, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLet AssociatedURLLoader listen to destruction of the Document used for loading
The DocumentThreadableLoader instance used by AssociatedURLLoader must
stop using objects on Oilpan heap before blink::shutdown() is called.
This CL adds a small helper class to observe destruction of the Document
associated with the passed frame by using ContextLifecycleObserver.
I chose to use a small helper class than making AssociatedURLLoader as
it requires a lot of changes to make the users hold it using Persistent.
Since ContextLifecycleObserver::contextDestoryed() won't be invoked for
some kind of Documents, in such case, we crash the process by checking
ThreadState::current().
BUG=588566
R=yhirano,haraken
Committed: https://crrev.com/7a247ee1edbb291fbbb5377a3b2073e1e83ce803
Cr-Commit-Position: refs/heads/master@{#386044}
Patch Set 1 #Patch Set 2 : Prevent documentStopped() call after destruction of AssociatedURLLoader #
Total comments: 4
Patch Set 3 : Addressed #10 and cleaned up code #Patch Set 4 : Use ContextLifecycleObserver than DocumentLifecycleObserver #Patch Set 5 : Fixed crash #
Total comments: 1
Patch Set 6 : Add release assert #Patch Set 7 : Added comment to RELEASE_ASSERT #
Total comments: 6
Patch Set 8 : Addressed #38 #Patch Set 9 : Addressed #37 #
Messages
Total messages: 51 (19 generated)
The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run
Description was changed from ========== Let AssociatedURLLoader listen to destruction of the Document used for loading The DocumentThreadableLoader instance used by AssociatedURLLoader must stop using objects on Oilpan heap before blink::shutdown() is called. This CL adds a small helper class to observe destruction of the Document associated with the passed frame by using ActiveDOMObject. I chose to use a small helper class than making AssociatedURLLoader as it requires a lot of changes to make the users hold it using Persistent. BUG=588566 ========== to ========== Let AssociatedURLLoader listen to destruction of the Document used for loading The DocumentThreadableLoader instance used by AssociatedURLLoader must stop using objects on Oilpan heap before blink::shutdown() is called. This CL adds a small helper class to observe destruction of the Document associated with the passed frame by using ActiveDOMObject. I chose to use a small helper class than making AssociatedURLLoader as it requires a lot of changes to make the users hold it using Persistent. BUG=588566 ==========
tyoshino@chromium.org changed reviewers: + yhirano@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862073002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862073002/1
The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862073002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862073002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/1862073002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/AssociatedURLLoader.cpp (right): https://codereview.chromium.org/1862073002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/AssociatedURLLoader.cpp:291: class AssociatedURLLoader::DocumentWatcher final : public GarbageCollectedFinalized<DocumentWatcher>, public ActiveDOMObject { My understanding is ContextLifecycleObserver is more preferable than ActiveDOMObeject. https://bugs.chromium.org/p/chromium/issues/detail?id=589507 https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/UwRGJgLBElo/... https://codereview.chromium.org/1862073002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/AssociatedURLLoader.cpp:426: void AssociatedURLLoader::documentStopped() Do we need to detach the watcher when a loading finishes?
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862073002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862073002/40001
https://codereview.chromium.org/1862073002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/AssociatedURLLoader.cpp (right): https://codereview.chromium.org/1862073002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/AssociatedURLLoader.cpp:291: class AssociatedURLLoader::DocumentWatcher final : public GarbageCollectedFinalized<DocumentWatcher>, public ActiveDOMObject { On 2016/04/07 01:46:41, yhirano wrote: > My understanding is ContextLifecycleObserver is more preferable than > ActiveDOMObeject. https://bugs.chromium.org/p/chromium/issues/detail?id=589507 > https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/UwRGJgLBElo/... OK. Done https://codereview.chromium.org/1862073002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/AssociatedURLLoader.cpp:426: void AssociatedURLLoader::documentStopped() On 2016/04/07 01:46:42, yhirano wrote: > Do we need to detach the watcher when a loading finishes? Good catch! Done
The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862073002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862073002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)
I'll investigate the oilpan crash.
The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862073002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862073002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PS5 crashes in my envornment:
==36966==ERROR: AddressSanitizer: SEGV on unknown address 0x7e8e3aecf680 (pc
0x0001129580dc bp 0x7fff57452560 sp 0x7fff574524e0 T0)
==36966==The signal is caused by a UNKNOWN memory access.
SCARINESS: 25 (wild-addr)
#0 0x1129580db in blink::AssociatedURLLoader::cancel
AssociatedURLLoader.cpp:304
#1 0x11a648a26 in content::ResourceFetcherImpl::~ResourceFetcherImpl
resource_fetcher_impl.cc:34
#2 0x11a648b1d in content::ResourceFetcherImpl::~ResourceFetcherImpl
resource_fetcher_impl.cc:32
#3 0x11a648437 in
content::MultiResolutionImageResourceFetcher::~MultiResolutionImageResourceFetcher
memory:2545
#4 0x11a6943a9 in void
STLDeleteElements<std::__1::vector<content::MultiResolutionImageResourceFetcher*,
std::__1::allocator<content::MultiResolutionImageResourceFetcher*> > >
stl_util.h:44
#5 0x11a6900a8 in content::ImageDownloaderImpl::~ImageDownloaderImpl
scoped_vector.h:42
#6 0x110908b35 in mojo::internal::Router::OnConnectionError callback.h:83
#7 0x1108eab33 in mojo::internal::Connector::HandleError callback.h:83
#8 0x1108ebbb5 in mojo::internal::Connector::OnWatcherHandleReady
connector.cc:233
#9 0x110918910 in
mojo::Watcher::MessageLoopObserver::WillDestroyCurrentMessageLoop callback.h:397
#10 0x10cec4404 in base::MessageLoop::~MessageLoop message_loop.cc:171
#11 0x10cec595d in base::MessageLoop::~MessageLoop message_loop.cc:136
#12 0x11a7d189a in content::RenderThreadImpl::Shutdown memory:2545
#13 0x118480f26 in content::ChildProcess::~ChildProcess child_process.cc:71
#14 0x11a8665b0 in content::RendererMain renderer_main.cc:230
#15 0x10cd76610 in content::ContentMainRunnerImpl::Run
content_main_runner.cc:741
#16 0x10cd7452d in content::ContentMain content_main.cc:19
#17 0x10be0835f in ChromeMain chrome_main.cc:84
#18 0x1087ab99f in main chrome_exe_main_mac.c:87
#19 0x1087ab563 in start (in Chromium Helper) + 51
#20 0x12 (<unknown module>)
AddressSanitizer can not provide additional info.
https://codereview.chromium.org/1862073002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/AssociatedURLLoader.h (right): https://codereview.chromium.org/1862073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/AssociatedURLLoader.h:60: // Called by |m_observer| to handle destruction of the Document associated [optional] You can make these functions private if you like.
On 2016/04/07 10:26:40, yhirano1 wrote: > PS5 crashes in my envornment: > > ==36966==ERROR: AddressSanitizer: SEGV on unknown address 0x7e8e3aecf680 (pc > 0x0001129580dc bp 0x7fff57452560 sp 0x7fff574524e0 T0) > ==36966==The signal is caused by a UNKNOWN memory access. > SCARINESS: 25 (wild-addr) > #0 0x1129580db in blink::AssociatedURLLoader::cancel > AssociatedURLLoader.cpp:304 Hmm, this implies that the contextDestroyed hooking is not working. Could you please insert a log in the code to see if it's really invoked?
yhirano@, please test PS6
The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862073002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862073002/100001
Description was changed from ========== Let AssociatedURLLoader listen to destruction of the Document used for loading The DocumentThreadableLoader instance used by AssociatedURLLoader must stop using objects on Oilpan heap before blink::shutdown() is called. This CL adds a small helper class to observe destruction of the Document associated with the passed frame by using ActiveDOMObject. I chose to use a small helper class than making AssociatedURLLoader as it requires a lot of changes to make the users hold it using Persistent. BUG=588566 ========== to ========== Let AssociatedURLLoader listen to destruction of the Document used for loading The DocumentThreadableLoader instance used by AssociatedURLLoader must stop using objects on Oilpan heap before blink::shutdown() is called. This CL adds a small helper class to observe destruction of the Document associated with the passed frame by using ContextLifecycleObserver. I chose to use a small helper class than making AssociatedURLLoader as it requires a lot of changes to make the users hold it using Persistent. Since ContextLifecycleObserver::contextDestoryed() won't be invoked for some kind of Documents, in such case, we crash the process by checking ThreadState::current(). BUG=588566 ==========
Description was changed from ========== Let AssociatedURLLoader listen to destruction of the Document used for loading The DocumentThreadableLoader instance used by AssociatedURLLoader must stop using objects on Oilpan heap before blink::shutdown() is called. This CL adds a small helper class to observe destruction of the Document associated with the passed frame by using ContextLifecycleObserver. I chose to use a small helper class than making AssociatedURLLoader as it requires a lot of changes to make the users hold it using Persistent. Since ContextLifecycleObserver::contextDestoryed() won't be invoked for some kind of Documents, in such case, we crash the process by checking ThreadState::current(). BUG=588566 ========== to ========== Let AssociatedURLLoader listen to destruction of the Document used for loading The DocumentThreadableLoader instance used by AssociatedURLLoader must stop using objects on Oilpan heap before blink::shutdown() is called. This CL adds a small helper class to observe destruction of the Document associated with the passed frame by using ContextLifecycleObserver. I chose to use a small helper class than making AssociatedURLLoader as it requires a lot of changes to make the users hold it using Persistent. Since ContextLifecycleObserver::contextDestoryed() won't be invoked for some kind of Documents, in such case, we crash the process by checking ThreadState::current(). BUG=588566 ==========
tyoshino@chromium.org changed reviewers: + haraken@chromium.org
+haraken yhirano@ confirmed that this fix works by following the Clusterfuzz reproduction steps.
I saw no UAF-reports with PS6.
Adding comments at RELEASE_ASSERT would be good.
On 2016/04/08 07:14:09, yhirano wrote: > Adding comments at RELEASE_ASSERT would be good. Done
I chose to preserve the Observer code as for most of Documents it should just work without crashing the process.
lgtm https://codereview.chromium.org/1862073002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/AssociatedURLLoader.h (right): https://codereview.chromium.org/1862073002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/AssociatedURLLoader.h:77: // A DocumentLifecycleObserver for cancelling |m_loader| when the Document ContextLifecycleObserver
https://codereview.chromium.org/1862073002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/AssociatedURLLoader.cpp (right): https://codereview.chromium.org/1862073002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/AssociatedURLLoader.cpp:443: // The method of detecting Document destruction implemented here doesn't Add TODO. https://codereview.chromium.org/1862073002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/AssociatedURLLoader.cpp:445: // Oilpan is destroyed, we just crash the renderer process to prevent UaF. Sorry, how does this cause UaF?
https://codereview.chromium.org/1862073002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/AssociatedURLLoader.cpp (right): https://codereview.chromium.org/1862073002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/AssociatedURLLoader.cpp:443: // The method of detecting Document destruction implemented here doesn't On 2016/04/08 07:34:13, haraken wrote: > > Add TODO. Done. https://codereview.chromium.org/1862073002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/AssociatedURLLoader.cpp:445: // Oilpan is destroyed, we just crash the renderer process to prevent UaF. On 2016/04/08 07:34:13, haraken wrote: > > Sorry, how does this cause UaF? m_observer is on-heap. If the Oilpan is already shutdown, we shouldn't touch m_observer. We could consider the way that we just ignore the rest of the algorithm. But the fact that we were not notified of destruction of the context means that we haven't cleaned up the ThreadableLoader. ThreadableLoader involves some other non-Blink instances e.g. WebURLLoader. It may be still facing the on-heap objects, so we should kill the process for safety. Improved the comment.
https://codereview.chromium.org/1862073002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/AssociatedURLLoader.h (right): https://codereview.chromium.org/1862073002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/AssociatedURLLoader.h:77: // A DocumentLifecycleObserver for cancelling |m_loader| when the Document On 2016/04/08 07:24:02, yhirano wrote: > ContextLifecycleObserver Done.
Confirmed that the Document instance is created by using DOMImplementation::createDocument() which is mentioned as an example in the comment in Document::detach() explaining in which case contextDestroyed() is not invoked.
LGTM as a short-term fix. However, I don't think this is a right fix -- I think content/ should have logic to clear all running loaders before calling blink::shutdown (without relying on ContextLifecycleObservers).
Description was changed from ========== Let AssociatedURLLoader listen to destruction of the Document used for loading The DocumentThreadableLoader instance used by AssociatedURLLoader must stop using objects on Oilpan heap before blink::shutdown() is called. This CL adds a small helper class to observe destruction of the Document associated with the passed frame by using ContextLifecycleObserver. I chose to use a small helper class than making AssociatedURLLoader as it requires a lot of changes to make the users hold it using Persistent. Since ContextLifecycleObserver::contextDestoryed() won't be invoked for some kind of Documents, in such case, we crash the process by checking ThreadState::current(). BUG=588566 ========== to ========== Let AssociatedURLLoader listen to destruction of the Document used for loading The DocumentThreadableLoader instance used by AssociatedURLLoader must stop using objects on Oilpan heap before blink::shutdown() is called. This CL adds a small helper class to observe destruction of the Document associated with the passed frame by using ContextLifecycleObserver. I chose to use a small helper class than making AssociatedURLLoader as it requires a lot of changes to make the users hold it using Persistent. Since ContextLifecycleObserver::contextDestoryed() won't be invoked for some kind of Documents, in such case, we crash the process by checking ThreadState::current(). BUG=588566 R=yhirano,haraken ==========
The CQ bit was checked by tyoshino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/1862073002/#ps160001 (title: "Addressed #37")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862073002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862073002/160001
Message was sent while issue was closed.
Description was changed from ========== Let AssociatedURLLoader listen to destruction of the Document used for loading The DocumentThreadableLoader instance used by AssociatedURLLoader must stop using objects on Oilpan heap before blink::shutdown() is called. This CL adds a small helper class to observe destruction of the Document associated with the passed frame by using ContextLifecycleObserver. I chose to use a small helper class than making AssociatedURLLoader as it requires a lot of changes to make the users hold it using Persistent. Since ContextLifecycleObserver::contextDestoryed() won't be invoked for some kind of Documents, in such case, we crash the process by checking ThreadState::current(). BUG=588566 R=yhirano,haraken ========== to ========== Let AssociatedURLLoader listen to destruction of the Document used for loading The DocumentThreadableLoader instance used by AssociatedURLLoader must stop using objects on Oilpan heap before blink::shutdown() is called. This CL adds a small helper class to observe destruction of the Document associated with the passed frame by using ContextLifecycleObserver. I chose to use a small helper class than making AssociatedURLLoader as it requires a lot of changes to make the users hold it using Persistent. Since ContextLifecycleObserver::contextDestoryed() won't be invoked for some kind of Documents, in such case, we crash the process by checking ThreadState::current(). BUG=588566 R=yhirano,haraken ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Let AssociatedURLLoader listen to destruction of the Document used for loading The DocumentThreadableLoader instance used by AssociatedURLLoader must stop using objects on Oilpan heap before blink::shutdown() is called. This CL adds a small helper class to observe destruction of the Document associated with the passed frame by using ContextLifecycleObserver. I chose to use a small helper class than making AssociatedURLLoader as it requires a lot of changes to make the users hold it using Persistent. Since ContextLifecycleObserver::contextDestoryed() won't be invoked for some kind of Documents, in such case, we crash the process by checking ThreadState::current(). BUG=588566 R=yhirano,haraken ========== to ========== Let AssociatedURLLoader listen to destruction of the Document used for loading The DocumentThreadableLoader instance used by AssociatedURLLoader must stop using objects on Oilpan heap before blink::shutdown() is called. This CL adds a small helper class to observe destruction of the Document associated with the passed frame by using ContextLifecycleObserver. I chose to use a small helper class than making AssociatedURLLoader as it requires a lot of changes to make the users hold it using Persistent. Since ContextLifecycleObserver::contextDestoryed() won't be invoked for some kind of Documents, in such case, we crash the process by checking ThreadState::current(). BUG=588566 R=yhirano,haraken Committed: https://crrev.com/7a247ee1edbb291fbbb5377a3b2073e1e83ce803 Cr-Commit-Position: refs/heads/master@{#386044} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/7a247ee1edbb291fbbb5377a3b2073e1e83ce803 Cr-Commit-Position: refs/heads/master@{#386044}
Message was sent while issue was closed.
On 2016/04/08 09:29:31, haraken wrote: > LGTM as a short-term fix. > > However, I don't think this is a right fix -- I think content/ should have logic > to clear all running loaders before calling blink::shutdown (without relying on > ContextLifecycleObservers). Thanks. I agree. Spawned http://crbug.com/601737 for long-term fix. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
