Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2249173003:
Removes the references to shared workers from the all documents in being deleted frames (Closed)
|
Created:
4 years, 4 months ago by horo Modified:
4 years, 3 months ago CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, kinuko+watch, blink-worker-reviews_chromium.org, pucchakayala, Ken Russell (switch to Gerrit) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemoves the references to shared workers from the all documents in being deleted frames.
So that we can shut down any shared workers that are no longer referenced by active documents without waiting for DocumentDetached IPC message.
This will fix the race condition reported in https://crbug.com/636377#c11
BUG=639193
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/798a8b94726d3c439e6302998f31bc487f521ec8
Cr-Commit-Position: refs/heads/master@{#414427}
Patch Set 1 #Patch Set 2 : posttask #Patch Set 3 : Remove DCHECK_CURRENTLY_ON #Patch Set 4 : add comments #Patch Set 5 : Keep WorkerStoragePartitionId in SharedWorkerMessageFilter #
Total comments: 10
Patch Set 6 : incorporated nhiroki's comment #
Total comments: 4
Patch Set 7 : incorporated nhiroki's comment #Patch Set 8 : Removes the references when frames are being deleted #
Total comments: 2
Patch Set 9 : use WebContentsObserver::RenderFrameDeleted #
Total comments: 6
Patch Set 10 : incorporated slevers's comment #Patch Set 11 : reverted to Patch Set 8 #
Total comments: 2
Patch Set 12 : document_set_.erase(i++) #
Total comments: 6
Patch Set 13 : incorporated nhiroki's comment #Messages
Total messages: 92 (60 generated)
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [WIP] DetachAllDocument BUG= ========== to ========== DetachAllDocument when |isteners_| is empty in RenderProcessHostImpl::Cleanup() If there is no listeners, we can remove the reference from the all documents in the renderer process to shared workers without waiting DocumentDetached IPC message. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=636377 ==========
horo@chromium.org changed reviewers: + nhiroki@chromium.org
nhiroki@ Could you please review this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2249173003/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2249173003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:1934: // the renderer process to shared workers without waiting DocumentDetached IPC waiting -> waiting for https://codereview.chromium.org/2249173003/diff/80001/content/browser/shared_... File content/browser/shared_worker/shared_worker_host.h (right): https://codereview.chromium.org/2249173003/diff/80001/content/browser/shared_... content/browser/shared_worker/shared_worker_host.h:63: void DetachAllDocument(SharedWorkerMessageFilter* filter); AllDocument -> AllDocuments "Removes the references to shared workers from ..." could be a bit more readable. https://codereview.chromium.org/2249173003/diff/80001/content/browser/shared_... File content/browser/shared_worker/shared_worker_message_filter.cc (left): https://codereview.chromium.org/2249173003/diff/80001/content/browser/shared_... content/browser/shared_worker/shared_worker_message_filter.cc:47: DCHECK_CURRENTLY_ON(BrowserThread::IO); Why did you remove this? https://codereview.chromium.org/2249173003/diff/80001/content/browser/shared_... File content/browser/shared_worker/shared_worker_message_filter.h (right): https://codereview.chromium.org/2249173003/diff/80001/content/browser/shared_... content/browser/shared_worker/shared_worker_message_filter.h:40: void DetachAllDocument(); ditto. https://codereview.chromium.org/2249173003/diff/80001/content/browser/shared_... File content/browser/shared_worker/shared_worker_service_impl.h (right): https://codereview.chromium.org/2249173003/diff/80001/content/browser/shared_... content/browser/shared_worker/shared_worker_service_impl.h:92: void DetachAllDocument(SharedWorkerMessageFilter* filter); ditto
https://codereview.chromium.org/2249173003/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2249173003/diff/80001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:1934: // the renderer process to shared workers without waiting DocumentDetached IPC On 2016/08/17 06:58:03, nhiroki wrote: > waiting -> waiting for Done. https://codereview.chromium.org/2249173003/diff/80001/content/browser/shared_... File content/browser/shared_worker/shared_worker_host.h (right): https://codereview.chromium.org/2249173003/diff/80001/content/browser/shared_... content/browser/shared_worker/shared_worker_host.h:63: void DetachAllDocument(SharedWorkerMessageFilter* filter); On 2016/08/17 06:58:03, nhiroki wrote: > AllDocument -> AllDocuments > > "Removes the references to shared workers from ..." could be a bit more > readable. Done. https://codereview.chromium.org/2249173003/diff/80001/content/browser/shared_... File content/browser/shared_worker/shared_worker_message_filter.cc (left): https://codereview.chromium.org/2249173003/diff/80001/content/browser/shared_... content/browser/shared_worker/shared_worker_message_filter.cc:47: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2016/08/17 06:58:04, nhiroki wrote: > Why did you remove this? SharedWorkerMessageFilter may be deleted on UI thread because RenderProcessHostImpl will have scoped_refptr<SharedWorkerMessageFilter>. I think it is ok to delete SharedWorkerMessageFilter on UI thread. https://codereview.chromium.org/2249173003/diff/80001/content/browser/shared_... File content/browser/shared_worker/shared_worker_message_filter.h (right): https://codereview.chromium.org/2249173003/diff/80001/content/browser/shared_... content/browser/shared_worker/shared_worker_message_filter.h:40: void DetachAllDocument(); On 2016/08/17 06:58:04, nhiroki wrote: > ditto. Done. https://codereview.chromium.org/2249173003/diff/80001/content/browser/shared_... File content/browser/shared_worker/shared_worker_service_impl.h (right): https://codereview.chromium.org/2249173003/diff/80001/content/browser/shared_... content/browser/shared_worker/shared_worker_service_impl.h:92: void DetachAllDocument(SharedWorkerMessageFilter* filter); On 2016/08/17 06:58:04, nhiroki wrote: > ditto Done.
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM s/isteners_/listeners_/ in the CL subject and description https://codereview.chromium.org/2249173003/diff/100001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2249173003/diff/100001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1930: // the renderer process to shared workers without waiting for DocumentDetached "the reference to shared workers" ? https://codereview.chromium.org/2249173003/diff/100001/content/browser/shared... File content/browser/shared_worker/shared_worker_service_impl.h (right): https://codereview.chromium.org/2249173003/diff/100001/content/browser/shared... content/browser/shared_worker/shared_worker_service_impl.h:90: // renderer process which is associated with |filter_| . And shuts down any There is an extra space after |filter|
Description was changed from ========== DetachAllDocument when |isteners_| is empty in RenderProcessHostImpl::Cleanup() If there is no listeners, we can remove the reference from the all documents in the renderer process to shared workers without waiting DocumentDetached IPC message. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=636377 ========== to ========== DetachAllDocument when |listeners_| is empty in RenderProcessHostImpl::Cleanup() If there is no listeners, we can remove the reference from the all documents in the renderer process to shared workers without waiting DocumentDetached IPC message. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=636377 ==========
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2249173003/diff/100001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2249173003/diff/100001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1930: // the renderer process to shared workers without waiting for DocumentDetached On 2016/08/17 07:34:06, nhiroki wrote: > "the reference to shared workers" ? Done. https://codereview.chromium.org/2249173003/diff/100001/content/browser/shared... File content/browser/shared_worker/shared_worker_service_impl.h (right): https://codereview.chromium.org/2249173003/diff/100001/content/browser/shared... content/browser/shared_worker/shared_worker_service_impl.h:90: // renderer process which is associated with |filter_| . And shuts down any On 2016/08/17 07:34:06, nhiroki wrote: > There is an extra space after |filter| Done.
horo@chromium.org changed reviewers: + kbr@chromium.org
kbr@ Could you please review content/browser/renderer_host/render_process_host_impl.*?
+CC: alokp@, pucchakayala@
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
horo@chromium.org changed reviewers: - kbr@chromium.org
kbr@ Ah sorry, this CL is not related to GPU-related stuff.
horo@chromium.org changed reviewers: + sievers@chromium.org
sievers@ Could you please review content/browser/renderer_host/render_process_host_impl.*?
On 2016/08/17 15:37:07, horo wrote: > sievers@ > Could you please review > content/browser/renderer_host/render_process_host_impl.*? It sounds like some problem where we don't handle widgets being torn down from the browser side (but expect the renderer to send an explicit msg). But why is the fix specific to the RenderProcessHost tearing things down when *all* listeners have gone away instead of each instance in the browser cleaning up things independently?
Description was changed from ========== DetachAllDocument when |listeners_| is empty in RenderProcessHostImpl::Cleanup() If there is no listeners, we can remove the reference from the all documents in the renderer process to shared workers without waiting DocumentDetached IPC message. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=636377 ========== to ========== Removes the references to shared workers from the all documents in being deleted frames. So that we can shut down any shared workers that are no longer referenced by active documents. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=636377 ==========
Description was changed from ========== Removes the references to shared workers from the all documents in being deleted frames. So that we can shut down any shared workers that are no longer referenced by active documents. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=636377 ========== to ========== Removes the references to shared workers from the all documents are being deleted frames. So that we can shut down any shared workers that are no longer referenced by active documents. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=636377 ==========
Description was changed from ========== Removes the references to shared workers from the all documents are being deleted frames. So that we can shut down any shared workers that are no longer referenced by active documents. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=636377 ========== to ========== Removes the references to shared workers from the all documents in being deleted frames. So that we can shut down any shared workers that are no longer referenced by active documents. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=636377 ==========
Description was changed from ========== Removes the references to shared workers from the all documents in being deleted frames. So that we can shut down any shared workers that are no longer referenced by active documents. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=636377 ========== to ========== DetachAllDocument when |listeners_| is empty in RenderProcessHostImpl::Cleanup() If there is no listeners, we can remove the reference from the all documents in the renderer process to shared workers without waiting DocumentDetached IPC message. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=636377 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== DetachAllDocument when |listeners_| is empty in RenderProcessHostImpl::Cleanup() If there is no listeners, we can remove the reference from the all documents in the renderer process to shared workers without waiting DocumentDetached IPC message. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=636377 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== DetachAllDocument when |listeners_| is empty in RenderProcessHostImpl::Cleanup() If there is no listeners, we can remove the reference from the all documents in the renderer process to shared workers without waiting for DocumentDetached IPC message. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=636377 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== DetachAllDocument when |listeners_| is empty in RenderProcessHostImpl::Cleanup() If there is no listeners, we can remove the reference from the all documents in the renderer process to shared workers without waiting for DocumentDetached IPC message. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=636377 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Removes the references to shared workers from the all documents in being deleted frames. So that we can shut down any shared workers that are no longer referenced by active documents without waiting for DocumentDetached IPC message. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=636377 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> It sounds like some problem where we don't handle widgets being torn down from
> the browser side (but expect the renderer to send an explicit msg).
Even if the renderer process is not responseive,
RenderWidgetHostImpl::RendererIsUnresponsive() will detele the
RenderFrameHostImpl.
TimeoutMonitor::CheckTimedOut()
RenderWidgetHostImpl::RendererIsUnresponsive()
WebContentsImpl::RendererUnresponsive()
WebContentsImpl::Close()
WebContentsImpl::Close()
Browser::CloseContents()
UnloadController::CanCloseContents()
UnloadController::ClearUnloadState()
UnloadController::ProcessPendingTabs()
Browser::OnWindowClosing()
TabStripModel::CloseAllTabs()
TabStripModel::InternalCloseTabs()
TabStripModel::InternalCloseTab()
WebContentsImpl::~WebContentsImpl()
FrameTree::~FrameTree()
FrameTreeNode::~FrameTreeNode()
RenderFrameHostManager::~RenderFrameHostManager()
RenderFrameHostImpl::~RenderFrameHostImpl()
RenderProcessHostImpl::RemoveRoute()
RenderProcessHostImpl::Cleanup()
> But why is the fix specific to the RenderProcessHost tearing things down when
> *all* listeners have gone away instead of each instance in the browser
cleaning
> up things independently?
Ah yes.
Tearing things down from the destructor of RenderFrameHostImpl sounds good.
I uploaded Patch Set 8.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Removes the references to shared workers from the all documents in being deleted frames. So that we can shut down any shared workers that are no longer referenced by active documents without waiting for DocumentDetached IPC message. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=636377 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Removes the references to shared workers from the all documents in being deleted frames. So that we can shut down any shared workers that are no longer referenced by active documents without waiting for DocumentDetached IPC message. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=639193 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
sievers@ Could you please review Patch Set 8?
alokp@chromium.org changed reviewers: + alokp@chromium.org
horo@: Thanks for this patch. Would you mind committing the instrumentation back (which was reverted recently) to ensure that this fix works as intended: https://chromium.googlesource.com/chromium/src/+/529173d1bbeb53b3f9438648fad2...
https://codereview.chromium.org/2249173003/diff/140001/content/browser/shared... File content/browser/shared_worker/shared_worker_host.h (right): https://codereview.chromium.org/2249173003/diff/140001/content/browser/shared... content/browser/shared_worker/shared_worker_host.h:63: void RenderFrameDetached(int render_process_id, int render_frame_id); Could this even just implemenent WebContentsObserver::RenderFrameDeleted()?
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2249173003/diff/140001/content/browser/shared... File content/browser/shared_worker/shared_worker_host.h (right): https://codereview.chromium.org/2249173003/diff/140001/content/browser/shared... content/browser/shared_worker/shared_worker_host.h:63: void RenderFrameDetached(int render_process_id, int render_frame_id); On 2016/08/22 21:13:37, sievers wrote: > Could this even just implemenent WebContentsObserver::RenderFrameDeleted()? Done.
On 2016/08/22 17:07:40, alokp wrote: > horo@: Thanks for this patch. > > Would you mind committing the instrumentation back (which was reverted recently) > to ensure that this fix works as intended: > https://chromium.googlesource.com/chromium/src/+/529173d1bbeb53b3f9438648fad2... OK, I will reland the patch after this CL.
nhiroki@ Could you plase review Patch Set 9. It is drastically different from Patch Set 7 which you L-G-T-Med.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
https://codereview.chromium.org/2249173003/diff/160001/content/browser/shared... File content/browser/shared_worker/shared_worker_service_impl.cc (right): https://codereview.chromium.org/2249173003/diff/160001/content/browser/shared... content/browser/shared_worker/shared_worker_service_impl.cc:129: // content::WebContentsObserver overrides. I wonder if this still needs to also listen to WebContentsDestroyed() since the filter might outlive the WebContents and ~WebContentsObserver() would deref |web_contents_|. But then it would also mean that |this| leaks in that case. It's not totally obvious how this is all balanced (creating this from OnCreateWorker and deleting it from RFDeleted). Apologies if my suggestion made this more complicated, did it maybe? https://codereview.chromium.org/2249173003/diff/160001/content/browser/shared... content/browser/shared_worker/shared_worker_service_impl.cc:364: BrowserThread::PostTask( Is it possible that there is a pending RenderFrameHost destruction here that races this task posted to the UI thread and we miss it? https://codereview.chromium.org/2249173003/diff/160001/content/browser/shared... content/browser/shared_worker/shared_worker_service_impl.cc:524: remove_pending_instance_list.push_back(it.first); why not erase immediately (while post-incrementing the iterator)?
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2249173003/diff/160001/content/browser/shared... File content/browser/shared_worker/shared_worker_service_impl.cc (right): https://codereview.chromium.org/2249173003/diff/160001/content/browser/shared... content/browser/shared_worker/shared_worker_service_impl.cc:129: // content::WebContentsObserver overrides. On 2016/08/23 20:32:42, sievers wrote: > I wonder if this still needs to also listen to WebContentsDestroyed() since the > filter might outlive the WebContents and ~WebContentsObserver() would deref > |web_contents_|. Done. > But then it would also mean that |this| leaks in that case. > It's not totally obvious how this is all balanced (creating this from > OnCreateWorker and deleting it from RFDeleted). Apologies if my suggestion made > this more complicated, did it maybe? The fact that SharedWorkerService related classes live on IO thread makes things complicated. I think that calling NotifyRenderFrameDetachedOnIO() from ~RenderFrameHostImpl() (Patch Set 8) is simpler. If you agree, I'd like to go with Patch Set 8. https://codereview.chromium.org/2249173003/diff/160001/content/browser/shared... content/browser/shared_worker/shared_worker_service_impl.cc:364: BrowserThread::PostTask( On 2016/08/23 20:32:42, sievers wrote: > Is it possible that there is a pending RenderFrameHost destruction here that > races this task posted to the UI thread and we miss it? Yes. So I check "if (!render_frame_host)" in RegisterWebContentsObserverOnUI. https://codereview.chromium.org/2249173003/diff/160001/content/browser/shared... content/browser/shared_worker/shared_worker_service_impl.cc:524: remove_pending_instance_list.push_back(it.first); On 2016/08/23 20:32:42, sievers wrote: > why not erase immediately (while post-incrementing the iterator)? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
> > If you agree, I'd like to go with Patch Set 8. sounds good (modulo other comments). sorry about the detour!
The CQ bit was checked by horo@chromium.org to run a CQ dry run
On 2016/08/24 23:14:45, sievers wrote: > > > > If you agree, I'd like to go with Patch Set 8. > > sounds good (modulo other comments). sorry about the detour! Done
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2249173003/diff/200001/content/browser/shared... File content/browser/shared_worker/worker_document_set.cc (right): https://codereview.chromium.org/2249173003/diff/200001/content/browser/shared... content/browser/shared_worker/worker_document_set.cc:77: document_set_.erase(item_to_delete); same as: document_set_.erase(i++)
The CQ bit was checked by horo@chromium.org to run a CQ dry run
https://codereview.chromium.org/2249173003/diff/200001/content/browser/shared... File content/browser/shared_worker/worker_document_set.cc (right): https://codereview.chromium.org/2249173003/diff/200001/content/browser/shared... content/browser/shared_worker/worker_document_set.cc:77: document_set_.erase(item_to_delete); On 2016/08/24 23:44:37, sievers wrote: > same as: document_set_.erase(i++) Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
nhiroki@ Could you plase review Patch Set 12. It is drastically different from Patch Set 7 which you L-G-T-Med.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm with nits https://codereview.chromium.org/2249173003/diff/220001/content/browser/shared... File content/browser/shared_worker/shared_worker_host.cc (right): https://codereview.chromium.org/2249173003/diff/220001/content/browser/shared... content/browser/shared_worker/shared_worker_host.cc:139: // Walk all instances and remove the all documents in the frame from their all the documents https://codereview.chromium.org/2249173003/diff/220001/content/browser/shared... File content/browser/shared_worker/shared_worker_service_impl.h (right): https://codereview.chromium.org/2249173003/diff/220001/content/browser/shared... content/browser/shared_worker/shared_worker_service_impl.h:89: // Removes the references to shared workers from the all documents in the all the documents https://codereview.chromium.org/2249173003/diff/220001/content/browser/shared... File content/browser/shared_worker/worker_document_set.cc (right): https://codereview.chromium.org/2249173003/diff/220001/content/browser/shared... content/browser/shared_worker/worker_document_set.cc:73: document_set_.erase(i++); "i = document_set_.erase(i)" may work?
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2249173003/diff/220001/content/browser/shared... File content/browser/shared_worker/shared_worker_host.cc (right): https://codereview.chromium.org/2249173003/diff/220001/content/browser/shared... content/browser/shared_worker/shared_worker_host.cc:139: // Walk all instances and remove the all documents in the frame from their On 2016/08/25 09:38:19, nhiroki wrote: > all the documents Done. https://codereview.chromium.org/2249173003/diff/220001/content/browser/shared... File content/browser/shared_worker/shared_worker_service_impl.h (right): https://codereview.chromium.org/2249173003/diff/220001/content/browser/shared... content/browser/shared_worker/shared_worker_service_impl.h:89: // Removes the references to shared workers from the all documents in the On 2016/08/25 09:38:19, nhiroki wrote: > all the documents Done. https://codereview.chromium.org/2249173003/diff/220001/content/browser/shared... File content/browser/shared_worker/worker_document_set.cc (right): https://codereview.chromium.org/2249173003/diff/220001/content/browser/shared... content/browser/shared_worker/worker_document_set.cc:73: document_set_.erase(i++); On 2016/08/25 09:38:19, nhiroki wrote: > "i = document_set_.erase(i)" may work? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 horo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org, nhiroki@chromium.org Link to the patchset: https://codereview.chromium.org/2249173003/#ps240001 (title: "incorporated nhiroki's comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Removes the references to shared workers from the all documents in being deleted frames. So that we can shut down any shared workers that are no longer referenced by active documents without waiting for DocumentDetached IPC message. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=639193 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Removes the references to shared workers from the all documents in being deleted frames. So that we can shut down any shared workers that are no longer referenced by active documents without waiting for DocumentDetached IPC message. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=639193 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Removes the references to shared workers from the all documents in being deleted frames. So that we can shut down any shared workers that are no longer referenced by active documents without waiting for DocumentDetached IPC message. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=639193 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Removes the references to shared workers from the all documents in being deleted frames. So that we can shut down any shared workers that are no longer referenced by active documents without waiting for DocumentDetached IPC message. This will fix the race condition reported in https://crbug.com/636377#c11 BUG=639193 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/798a8b94726d3c439e6302998f31bc487f521ec8 Cr-Commit-Position: refs/heads/master@{#414427} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/798a8b94726d3c439e6302998f31bc487f521ec8 Cr-Commit-Position: refs/heads/master@{#414427}
Message was sent while issue was closed.
On 2016/08/25 14:06:46, commit-bot: I haz the power wrote: > Patchset 13 (id:??) landed as > https://crrev.com/798a8b94726d3c439e6302998f31bc487f521ec8 > Cr-Commit-Position: refs/heads/master@{#414427} Would you mind committing the instrumentation back (which was reverted recently) to ensure that this fix works as intended: https://chromium.googlesource.com/chromium/src/+/529173d1bbeb53b3f9438648fad2...
Message was sent while issue was closed.
On 2016/08/26 17:43:28, alokp wrote: > On 2016/08/25 14:06:46, commit-bot: I haz the power wrote: > > Patchset 13 (id:??) landed as > > https://crrev.com/798a8b94726d3c439e6302998f31bc487f521ec8 > > Cr-Commit-Position: refs/heads/master@{#414427} > > Would you mind committing the instrumentation back (which was reverted recently) > to ensure that this fix works as intended: > https://chromium.googlesource.com/chromium/src/+/529173d1bbeb53b3f9438648fad2... Relanded. https://codereview.chromium.org/2286003002/ |
