Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(222)

Issue 1853333003: [BlobAsync] Faster shortcuttin, make renderer controller leaky & alive. (Closed)

Created:
4 years, 8 months ago by dmurph
Modified:
4 years, 8 months ago
Reviewers:
kinuko, michaeln, ojan
CC:
chromium-reviews, darin-cc_chromium.org, jam
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[BlobAsync] Faster shortcuttin, make renderer controller leaky & alive. This should fix strange memory access crashes observed in the following bugs by doing the following things: 1. We make the controller a leaky lazy instance so it isn't destructed. 2. We make one less message loop hop before we call AddProcessRef, so hopefully our process won't be shutting down in the middle of a transfer. 3. We send the 'descriptions' as early as possible if we're below the IPC threshold, which should help some of the speed regressions. 4. We prevent sudden shutdowns (like tab bar closing) by using blink::Platform::current()->suddenTerminationChanged(false); BUG=600462, 600435, 599490, 599416, 375297 Committed: https://crrev.com/90774e39bb24b1c531e9b3aba16c77ab96675fb3 Cr-Commit-Position: refs/heads/master@{#385472}

Patch Set 1 #

Patch Set 2 : format #

Patch Set 3 : git cl owners #

Total comments: 5

Patch Set 4 : rebase #

Patch Set 5 : windows build #

Patch Set 6 : Comments #

Total comments: 10

Patch Set 7 : comments #

Patch Set 8 : Always sending start on main thread #

Patch Set 9 : comments #

Total comments: 1

Patch Set 10 : Disabling fast shutdown when transfering blobs #

Total comments: 4

Patch Set 11 : Added sudden shutdown protection in renderer #

Total comments: 2

Patch Set 12 : comment #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+216 lines, -139 lines) Patch
M content/browser/blob_storage/blob_async_builder_host_unittest.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +2 lines, -27 lines 0 comments Download
M content/browser/blob_storage/blob_dispatcher_host.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/blob_storage/blob_dispatcher_host.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +5 lines, -3 lines 0 comments Download
M content/browser/blob_storage/blob_dispatcher_host_unittest.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +42 lines, -0 lines 0 comments Download
M content/child/blob_storage/blob_transport_controller.h View 1 2 3 4 5 6 7 5 chunks +20 lines, -14 lines 0 comments Download
M content/child/blob_storage/blob_transport_controller.cc View 1 2 3 4 5 6 7 8 9 10 7 chunks +53 lines, -26 lines 1 comment Download
M content/child/blob_storage/blob_transport_controller_unittest.cc View 1 2 3 4 5 6 7 7 chunks +83 lines, -40 lines 0 comments Download
M content/child/webblobregistry_impl.h View 1 2 3 1 chunk +0 lines, -7 lines 0 comments Download
M content/child/webblobregistry_impl.cc View 1 2 3 4 5 6 7 1 chunk +3 lines, -16 lines 0 comments Download
M storage/browser/blob/blob_async_builder_host.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +6 lines, -3 lines 0 comments Download
M storage/browser/blob/blob_async_builder_host.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -3 lines 0 comments Download

Messages

Total messages: 32 (11 generated)
dmurph
Hello! There are some blob-related crashes and regressions, and this patch should fix those. The ...
4 years, 8 months ago (2016-04-04 21:27:09 UTC) #4
michaeln
lgtm https://codereview.chromium.org/1853333003/diff/40001/content/child/blob_storage/blob_transport_controller.cc File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1853333003/diff/40001/content/child/blob_storage/blob_transport_controller.cc#newcode65 content/child/blob_storage/blob_transport_controller.cc:65: IncChildProcessRefCount(); having to do this immediately makes sense ...
4 years, 8 months ago (2016-04-05 00:18:04 UTC) #5
dmurph
Whoops, I had the wrong Kinuko added. Kinuko, would you mind taking a look at ...
4 years, 8 months ago (2016-04-05 00:32:05 UTC) #7
dmurph
Ah, nevermind, I see what you mean w.r.t. the io thread sender. Huh. Maybe that ...
4 years, 8 months ago (2016-04-05 00:39:51 UTC) #8
michaeln
On 2016/04/05 00:39:51, dmurph wrote: > Ah, nevermind, I see what you mean w.r.t. the ...
4 years, 8 months ago (2016-04-05 01:13:32 UTC) #9
kinuko
https://codereview.chromium.org/1853333003/diff/100001/content/child/blob_storage/blob_transport_controller.cc File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1853333003/diff/100001/content/child/blob_storage/blob_transport_controller.cc#newcode78 content/child/blob_storage/blob_transport_controller.cc:78: io_runner->PostTask( Per comment this function could be called on ...
4 years, 8 months ago (2016-04-05 09:54:46 UTC) #10
dmurph
https://codereview.chromium.org/1853333003/diff/100001/content/child/blob_storage/blob_transport_controller.cc File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1853333003/diff/100001/content/child/blob_storage/blob_transport_controller.cc#newcode78 content/child/blob_storage/blob_transport_controller.cc:78: io_runner->PostTask( On 2016/04/05 at 09:54:45, kinuko wrote: > Per ...
4 years, 8 months ago (2016-04-05 18:24:24 UTC) #11
michaeln
https://codereview.chromium.org/1853333003/diff/100001/content/child/blob_storage/blob_transport_controller.cc File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1853333003/diff/100001/content/child/blob_storage/blob_transport_controller.cc#newcode83 content/child/blob_storage/blob_transport_controller.cc:83: base::Passed(std::move(sender)), presend_descriptions, On 2016/04/05 18:24:24, dmurph wrote: > On ...
4 years, 8 months ago (2016-04-05 18:49:58 UTC) #12
dmurph
https://codereview.chromium.org/1853333003/diff/100001/content/child/blob_storage/blob_transport_controller.cc File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1853333003/diff/100001/content/child/blob_storage/blob_transport_controller.cc#newcode83 content/child/blob_storage/blob_transport_controller.cc:83: base::Passed(std::move(sender)), presend_descriptions, On 2016/04/05 at 18:49:58, michaeln wrote: > ...
4 years, 8 months ago (2016-04-05 19:19:24 UTC) #13
michaeln
lgtm https://codereview.chromium.org/1853333003/diff/160001/content/child/blob_storage/blob_transport_controller.cc File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1853333003/diff/160001/content/child/blob_storage/blob_transport_controller.cc#newcode62 content/child/blob_storage/blob_transport_controller.cc:62: scoped_refptr<ThreadSafeSender> sender, since you no longer xfer or ...
4 years, 8 months ago (2016-04-05 19:25:25 UTC) #14
dmurph
Hello everyone! After talking with Ojan, it became clear that the current way I was ...
4 years, 8 months ago (2016-04-05 22:07:39 UTC) #17
michaeln
https://codereview.chromium.org/1853333003/diff/180001/content/child/blob_storage/blob_transport_controller.cc File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1853333003/diff/180001/content/child/blob_storage/blob_transport_controller.cc#newcode45 content/child/blob_storage/blob_transport_controller.cc:45: ChildProcess::current()->AddRefProcess(); good catch needing to fiddle suddenDeath, it'd be ...
4 years, 8 months ago (2016-04-05 22:27:05 UTC) #18
dmurph
Ok, so I moved the sudden shutdown prevention to the renderer, and I did a ...
4 years, 8 months ago (2016-04-05 22:54:34 UTC) #19
michaeln
Actually, I'm not sure this is needed? Here's what i'm thinking. If the ProcessRef doesn't ...
4 years, 8 months ago (2016-04-05 22:57:54 UTC) #20
michaeln
Ah... I see, it's needed because the tab strip calls FastShutdownForPageCount() when closing tabs.
4 years, 8 months ago (2016-04-05 23:57:04 UTC) #21
michaeln
https://codereview.chromium.org/1853333003/diff/200001/content/public/browser/render_process_host.h File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/1853333003/diff/200001/content/public/browser/render_process_host.h#newcode203 content/public/browser/render_process_host.h:203: // the same number of calls with 'true' to ...
4 years, 8 months ago (2016-04-06 01:03:46 UTC) #22
dmurph
https://codereview.chromium.org/1853333003/diff/200001/content/public/browser/render_process_host.h File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/1853333003/diff/200001/content/public/browser/render_process_host.h#newcode203 content/public/browser/render_process_host.h:203: // the same number of calls with 'true' to ...
4 years, 8 months ago (2016-04-06 01:45:40 UTC) #23
kinuko
lgtm https://codereview.chromium.org/1853333003/diff/220001/content/child/blob_storage/blob_transport_controller.cc File content/child/blob_storage/blob_transport_controller.cc (right): https://codereview.chromium.org/1853333003/diff/220001/content/child/blob_storage/blob_transport_controller.cc#newcode87 content/child/blob_storage/blob_transport_controller.cc:87: // TODO(dmurph): Merge register and start messages. Yeah ...
4 years, 8 months ago (2016-04-06 07:25:01 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1853333003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1853333003/220001
4 years, 8 months ago (2016-04-06 15:48:06 UTC) #29
commit-bot: I haz the power
Committed patchset #12 (id:220001)
4 years, 8 months ago (2016-04-06 16:20:53 UTC) #30
commit-bot: I haz the power
4 years, 8 months ago (2016-04-06 16:21:59 UTC) #32
Message was sent while issue was closed.
Patchset 12 (id:??) landed as
https://crrev.com/90774e39bb24b1c531e9b3aba16c77ab96675fb3
Cr-Commit-Position: refs/heads/master@{#385472}

Powered by Google App Engine
This is Rietveld 408576698