ServiceWorker: Fetch Response blob might get destructed before it's handled w/o MojoBlob |
|||||||||
Issue descriptionProactively filing this as it looks it's likely going to be an issue. The milestones that'd be affected are M62 and M63. I removed legacy IPC for Response Blob handling in in: https://chromium-review.googlesource.com/c/chromium/src/+/615847 Where I added code in service_worker_context_client like (abridged): if (response.blob) { // MojoBlob case: blob_ptr = response.blob->TakeBlobPtr(); response.blob = nullptr; } else { // Non-MojoBlob case: if (!blob_registry_) { GetBlobRegistry(MakeRequest(&blob_registry_)); } blob_registry_->GetBlobFromUUID(MakeRequest(&blob_ptr), response.blob_uuid); } It turns out that mojo::BlobRegistry is not available if MojoBlob is not supported (heh should have noticed). For Blob creation is't fine as the IPC is sync now, but Blob destruction is not guaranteed now. (Actually even when MojoBlob is supported GetBlobFromUUID is not sync (yet) so it could have ordering issue) I'll try to see if I can make as small fix as possible (so it can be merged if necessary). If you start to see suspicious errors around this please merge them.
,
Oct 27 2017
,
Oct 27 2017
It's landed a few days ago and it looks it's going well. The change's relatively simple and it's regression, could we merge?
,
Oct 27 2017
,
Oct 27 2017
This bug requires manual review: M63 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 27 2017
Is the change critical enough to merge to M63 or can it wait until M64? Note: M63 has already been promoted to the beta branch so merge bar is high.
,
Nov 1 2017
Ping!
,
Nov 1 2017
Sorry for the late response. We haven't observed anything critical yet, but problems could happen due to GC timing so it's hard to tell beforehand, and the feature itself is used by multiple high-value sites. To add one more point this one must have pretty low risk for merging.
,
Nov 1 2017
Approving merge to M63 branch 3239 based on comment #3 and #8. Please merge ASAP. Thank you.
,
Nov 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/415422e42b4395802fbf0ef93e4930ee7bf04f79 commit 415422e42b4395802fbf0ef93e4930ee7bf04f79 Author: Kinuko Yasuda <kinuko@chromium.org> Date: Thu Nov 02 13:30:03 2017 Merge-M63: Add a temporary fix for Blob lifetime with SW FetchEvent without MojoBlob Add a new method to ServiceWorkerFetchResponseCallback for legacy blob which calls back to the caller when it's done, and hold a (copy) of WebServiceWorkerResponse until it's fired. WebServiceWorkerResponse is a copyable wrapper object of scoped_refptr<WebServiceWorkerResponsePrivate>, which has RefPtr<BlobDataHandle> in its member. TBR=kinuko@chromium.org (cherry picked from commit 7640944da7ef7b5fa2e61165b9f317b9764e1161) Bug: 776642 Change-Id: I0d6fff4ca64236a3d6ed69e1790db2c41ccae226 Reviewed-on: https://chromium-review.googlesource.com/734135 Commit-Queue: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#511284} Reviewed-on: https://chromium-review.googlesource.com/750925 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/branch-heads/3239@{#345} Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578} [modify] https://crrev.com/415422e42b4395802fbf0ef93e4930ee7bf04f79/content/browser/service_worker/service_worker_fetch_dispatcher.cc [modify] https://crrev.com/415422e42b4395802fbf0ef93e4930ee7bf04f79/content/child/service_worker/service_worker_subresource_loader.cc [modify] https://crrev.com/415422e42b4395802fbf0ef93e4930ee7bf04f79/content/child/service_worker/service_worker_subresource_loader.h [modify] https://crrev.com/415422e42b4395802fbf0ef93e4930ee7bf04f79/content/common/service_worker/service_worker_fetch_response_callback.mojom [modify] https://crrev.com/415422e42b4395802fbf0ef93e4930ee7bf04f79/content/renderer/service_worker/service_worker_context_client.cc
,
Nov 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/13802e31c540e80248e04e8f262091a0767ce988 commit 13802e31c540e80248e04e8f262091a0767ce988 Author: Kinuko Yasuda <kinuko@chromium.org> Date: Thu Nov 02 15:08:28 2017 [Reland] Merge-M63: Add a temporary fix for Blob lifetime with SW FetchEvent without MojoBlob Add a new method to ServiceWorkerFetchResponseCallback for legacy blob which calls back to the caller when it's done, and hold a (copy) of WebServiceWorkerResponse until it's fired. WebServiceWorkerResponse is a copyable wrapper object of scoped_refptr<WebServiceWorkerResponsePrivate>, which has RefPtr<BlobDataHandle> in its member. (cherry picked from commit 7640944da7ef7b5fa2e61165b9f317b9764e1161) Bug: 776642 Change-Id: I0d6fff4ca64236a3d6ed69e1790db2c41ccae226 Reviewed-on: https://chromium-review.googlesource.com/734135 Commit-Queue: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#511284} Reviewed-on: https://chromium-review.googlesource.com/750925 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/branch-heads/3239@{#345} Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578} (cherry picked from commit 415422e42b4395802fbf0ef93e4930ee7bf04f79) Change-Id: I68fe200c3f45a91c4064cc4cd3a2dd25c1257aa2 Reviewed-on: https://chromium-review.googlesource.com/751222 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/branch-heads/3239@{#347} Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578} [modify] https://crrev.com/13802e31c540e80248e04e8f262091a0767ce988/content/browser/service_worker/browser_side_controller_service_worker.cc [modify] https://crrev.com/13802e31c540e80248e04e8f262091a0767ce988/content/browser/service_worker/service_worker_fetch_dispatcher.cc [modify] https://crrev.com/13802e31c540e80248e04e8f262091a0767ce988/content/child/service_worker/service_worker_subresource_loader.cc [modify] https://crrev.com/13802e31c540e80248e04e8f262091a0767ce988/content/child/service_worker/service_worker_subresource_loader.h [modify] https://crrev.com/13802e31c540e80248e04e8f262091a0767ce988/content/common/service_worker/service_worker_fetch_response_callback.mojom [modify] https://crrev.com/13802e31c540e80248e04e8f262091a0767ce988/content/renderer/service_worker/service_worker_context_client.cc
,
Nov 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e13b4e699084bd18cb4ded4a6c5d408c0b161555 commit e13b4e699084bd18cb4ded4a6c5d408c0b161555 Author: Kinuko Yasuda <kinuko@chromium.org> Date: Thu Nov 02 16:49:49 2017 [Reland-2] Merge-M63: Add a temporary fix for Blob lifetime with SW FetchEvent without MojoBlob Add a new method to ServiceWorkerFetchResponseCallback for legacy blob which calls back to the caller when it's done, and hold a (copy) of WebServiceWorkerResponse until it's fired. WebServiceWorkerResponse is a copyable wrapper object of scoped_refptr<WebServiceWorkerResponsePrivate>, which has RefPtr<BlobDataHandle> in its member. (cherry picked from commit 7640944da7ef7b5fa2e61165b9f317b9764e1161) Bug: 776642 Change-Id: I0d6fff4ca64236a3d6ed69e1790db2c41ccae226 Reviewed-on: https://chromium-review.googlesource.com/734135 Commit-Queue: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#511284} Reviewed-on: https://chromium-review.googlesource.com/750925 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/branch-heads/3239@{#345} Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578} (cherry picked from commit 415422e42b4395802fbf0ef93e4930ee7bf04f79) Change-Id: I68fe200c3f45a91c4064cc4cd3a2dd25c1257aa2 Reviewed-on: https://chromium-review.googlesource.com/751222 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/branch-heads/3239@{#347} Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578} (cherry picked from commit 13802e31c540e80248e04e8f262091a0767ce988) TBR=kinuko@chromium.org Change-Id: I5d269b558b9cf39d32bbc57c292ab26edfc2e1aa Reviewed-on: https://chromium-review.googlesource.com/751602 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/branch-heads/3239@{#350} Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578} [modify] https://crrev.com/e13b4e699084bd18cb4ded4a6c5d408c0b161555/content/browser/service_worker/browser_side_controller_service_worker.cc [modify] https://crrev.com/e13b4e699084bd18cb4ded4a6c5d408c0b161555/content/browser/service_worker/service_worker_fetch_dispatcher.cc [modify] https://crrev.com/e13b4e699084bd18cb4ded4a6c5d408c0b161555/content/child/service_worker/service_worker_subresource_loader.cc [modify] https://crrev.com/e13b4e699084bd18cb4ded4a6c5d408c0b161555/content/child/service_worker/service_worker_subresource_loader.h [modify] https://crrev.com/e13b4e699084bd18cb4ded4a6c5d408c0b161555/content/common/service_worker/service_worker_fetch_response_callback.mojom [modify] https://crrev.com/e13b4e699084bd18cb4ded4a6c5d408c0b161555/content/renderer/service_worker/service_worker_context_client.cc
,
Nov 2 2017
change listed at #12 is reverted as it broke M63 build (see bug 780912 for more details) Here is reverted cl - https://chromium.googlesource.com/chromium/src.git/+/a8c031b57609072f2830ddf3d8b36ba6d0cc435e. Also now I'm rejecting merge to M63 as it broke the build even after multiple attempts to merge at #10, #11 and #12. Please let me know if there is any concern here. Thank you.
,
Nov 2 2017
Sigh, sorry for the breakage. There were so many code churns that made this merge very tough.
,
Nov 9 2017
Fixed though it's not get merged. MojoBlob's going to be enabled in M64, let's remove the code after we make sure it sticks (e.g. in M65).
,
Jun 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cc57856f281cf1d7f5f3e3ead5c8d5cb313ee05d commit cc57856f281cf1d7f5f3e3ead5c8d5cb313ee05d Author: Kinuko Yasuda <kinuko@chromium.org> Date: Wed Jun 27 10:37:58 2018 Remove the remaining OnResponsesLegacyBlob code MojoBlob is default and most other code has been already removed. Bug: 776642 Change-Id: Ibf11da14a3dd46c7f4613d0617e0ee0c3b87b999 Reviewed-on: https://chromium-review.googlesource.com/1114564 Reviewed-by: Matt Falkenhagen <falken@chromium.org> Commit-Queue: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/heads/master@{#570725} [modify] https://crrev.com/cc57856f281cf1d7f5f3e3ead5c8d5cb313ee05d/content/browser/service_worker/service_worker_fetch_dispatcher.cc [modify] https://crrev.com/cc57856f281cf1d7f5f3e3ead5c8d5cb313ee05d/content/common/service_worker/service_worker_fetch_response_callback.mojom [modify] https://crrev.com/cc57856f281cf1d7f5f3e3ead5c8d5cb313ee05d/content/renderer/service_worker/service_worker_subresource_loader.cc [modify] https://crrev.com/cc57856f281cf1d7f5f3e3ead5c8d5cb313ee05d/content/renderer/service_worker/service_worker_subresource_loader.h |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by bugdroid1@chromium.org
, Oct 24 2017