Remove instrumentation added for ServiceWorkerObjectHost related crash |
||||||
Issue descriptionRemove the instrumentation from issue 838410 and issue 854993 as they are now fixed.
,
Jul 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7ec7b14676f9ce9a96a86a39d6d2c1f496c45585 commit 7ec7b14676f9ce9a96a86a39d6d2c1f496c45585 Author: Matt Falkenhagen <falken@chromium.org> Date: Mon Jul 30 10:16:45 2018 Revert "service worker: Remove instrumentation for ServiceWorkerObjectHost crash." This reverts commit bdff024cf35d3ca66c6f01c5dfaa38809d60a84a. Reason for revert: It turns out the crash was still happening up to 70.0.3501.2, before this was committed. The crash went away since 70.0.3502.0, but it's unclear whether that was because the instrumentation was removed or the "quick fix" was removed at the same time (r577435). I'm removing the instrumentation to see if the crashes will return. Original change's description: > service worker: Remove instrumentation for ServiceWorkerObjectHost crash. > > Remove the instrumentation from issue 854993 (and duped issue 838410) as > it is now fixed. > > This is mostly a straight revert of the CLs, but it retains some > checks as DCHECKs and other improvements like adding constness. > > Bug: 866769 , 854993 > Change-Id: Id617e06e85e2b947258ba5f80c1d7aa0396888af > Reviewed-on: https://chromium-review.googlesource.com/1147888 > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > Commit-Queue: Matt Falkenhagen <falken@chromium.org> > Cr-Commit-Position: refs/heads/master@{#577430} TBR=falken@chromium.org,kinuko@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 866769 , 854993 Change-Id: I169f6c894d862008f4cfb0b778cd6f657b8351c0 Reviewed-on: https://chromium-review.googlesource.com/1154740 Reviewed-by: Matt Falkenhagen <falken@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#579004} [modify] https://crrev.com/7ec7b14676f9ce9a96a86a39d6d2c1f496c45585/content/browser/service_worker/service_worker_object_host.cc [modify] https://crrev.com/7ec7b14676f9ce9a96a86a39d6d2c1f496c45585/content/browser/service_worker/service_worker_object_host.h [modify] https://crrev.com/7ec7b14676f9ce9a96a86a39d6d2c1f496c45585/content/browser/service_worker/service_worker_provider_host.cc [modify] https://crrev.com/7ec7b14676f9ce9a96a86a39d6d2c1f496c45585/content/browser/service_worker/service_worker_provider_host.h
,
Aug 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/45fabee294b62e4e1c7c7cc36d6a779d80d4c57a commit 45fabee294b62e4e1c7c7cc36d6a779d80d4c57a Author: Matt Falkenhagen <falken@chromium.org> Date: Mon Aug 20 08:51:59 2018 Reland "service worker: Remove instrumentation for ServiceWorkerObjectHost crash." This reverts commit 7ec7b14676f9ce9a96a86a39d6d2c1f496c45585. Reason for revert: The crash has now been fixed, really, by r580102 and r580149. Original change's description: > Revert "service worker: Remove instrumentation for ServiceWorkerObjectHost crash." > > This reverts commit bdff024cf35d3ca66c6f01c5dfaa38809d60a84a. > > Reason for revert: > It turns out the crash was still happening up to 70.0.3501.2, before > this was committed. The crash went away since 70.0.3502.0, but it's > unclear whether that was because the instrumentation was removed or > the "quick fix" was removed at the same time (r577435). > > I'm removing the instrumentation to see if the crashes will return. > > Original change's description: > > service worker: Remove instrumentation for ServiceWorkerObjectHost crash. > > > > Remove the instrumentation from issue 854993 (and duped issue 838410) as > > it is now fixed. > > > > This is mostly a straight revert of the CLs, but it retains some > > checks as DCHECKs and other improvements like adding constness. > > > > Bug: 866769 , 854993 > > Change-Id: Id617e06e85e2b947258ba5f80c1d7aa0396888af > > Reviewed-on: https://chromium-review.googlesource.com/1147888 > > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > > Commit-Queue: Matt Falkenhagen <falken@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#577430} > > TBR=falken@chromium.org,kinuko@chromium.org > > # Not skipping CQ checks because original CL landed > 1 day ago. > > Bug: 866769 , 854993 > Change-Id: I169f6c894d862008f4cfb0b778cd6f657b8351c0 > Reviewed-on: https://chromium-review.googlesource.com/1154740 > Reviewed-by: Matt Falkenhagen <falken@chromium.org> > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > Commit-Queue: Matt Falkenhagen <falken@chromium.org> > Cr-Commit-Position: refs/heads/master@{#579004} TBR=falken@chromium.org,kinuko@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 866769 , 854993 Change-Id: Ifacb8a30b851dc0d9808b26e01fecbd3dbbdb45d Reviewed-on: https://chromium-review.googlesource.com/1180901 Reviewed-by: Matt Falkenhagen <falken@chromium.org> Commit-Queue: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#584381} [modify] https://crrev.com/45fabee294b62e4e1c7c7cc36d6a779d80d4c57a/content/browser/service_worker/service_worker_object_host.cc [modify] https://crrev.com/45fabee294b62e4e1c7c7cc36d6a779d80d4c57a/content/browser/service_worker/service_worker_object_host.h [modify] https://crrev.com/45fabee294b62e4e1c7c7cc36d6a779d80d4c57a/content/browser/service_worker/service_worker_provider_host.cc [modify] https://crrev.com/45fabee294b62e4e1c7c7cc36d6a779d80d4c57a/content/browser/service_worker/service_worker_provider_host.h
,
Aug 21
Request merge of c#3 to M69. It removes debugging instrumentation which is used for content::ServiceWorkerObjectHost::CrashOnDoubleDelete crashes, which no longer occur on 69 since issue 854993 was fixed and merged in 69.0.3497.31: https://crash.corp.google.com/browse?q=product_name%3D%27Chrome%27+AND+expanded_custom_data.ChromeCrashProto.channel%3D%27beta%27+AND+expanded_custom_data.ChromeCrashProto.ptype%3D%27browser%27+AND+expanded_custom_data.ChromeCrashProto.magic_signature_1.name%3D%27content%3A%3AServiceWorkerObjectHost%3A%3ACrashOnDoubleDelete%27 It's not a huge deal if 69 Stable is released with the instrumentation but the browser would be doing unnecessary logging which could potentially impact performance.
,
Aug 21
This bug requires manual review: We are only 13 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 21
Is this M69 regression? If yes, how safe and critical is to merge cl listed at #3 to M69 this late in release cycle?
,
Aug 21
It's a regression in that the instrumentation is not in M68 Stable. Removing the code is safe. It just removes debug logging. Removing the code is not critical. It just removes debug logging that is unnecessary and possibly impacting performance. The reason the request is this late is because the crash bug was fixed in M69 Beta, and I left the debugging instrumentation in to ensure the bug was really fixed.
,
Aug 21
Approving merge to M69 branch 3497 based on comments #4 and #7. Pls merge. Thank you.
,
Aug 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cbaf6152a96439c6846f07c14811e09315506367 commit cbaf6152a96439c6846f07c14811e09315506367 Author: Matt Falkenhagen <falken@chromium.org> Date: Tue Aug 21 02:41:35 2018 M69: Reland "service worker: Remove instrumentation for ServiceWorkerObjectHost crash." This reverts commit 7ec7b14676f9ce9a96a86a39d6d2c1f496c45585. Reason for revert: The crash has now been fixed, really, by r580102 and r580149. Original change's description: > Revert "service worker: Remove instrumentation for ServiceWorkerObjectHost crash." > > This reverts commit bdff024cf35d3ca66c6f01c5dfaa38809d60a84a. > > Reason for revert: > It turns out the crash was still happening up to 70.0.3501.2, before > this was committed. The crash went away since 70.0.3502.0, but it's > unclear whether that was because the instrumentation was removed or > the "quick fix" was removed at the same time (r577435). > > I'm removing the instrumentation to see if the crashes will return. > > Original change's description: > > service worker: Remove instrumentation for ServiceWorkerObjectHost crash. > > > > Remove the instrumentation from issue 854993 (and duped issue 838410) as > > it is now fixed. > > > > This is mostly a straight revert of the CLs, but it retains some > > checks as DCHECKs and other improvements like adding constness. > > > > Bug: 866769 , 854993 > > Change-Id: Id617e06e85e2b947258ba5f80c1d7aa0396888af > > Reviewed-on: https://chromium-review.googlesource.com/1147888 > > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > > Commit-Queue: Matt Falkenhagen <falken@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#577430} > > TBR=falken@chromium.org,kinuko@chromium.org > > # Not skipping CQ checks because original CL landed > 1 day ago. > > Bug: 866769 , 854993 > Change-Id: I169f6c894d862008f4cfb0b778cd6f657b8351c0 > Reviewed-on: https://chromium-review.googlesource.com/1154740 > Reviewed-by: Matt Falkenhagen <falken@chromium.org> > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > Commit-Queue: Matt Falkenhagen <falken@chromium.org> > Cr-Commit-Position: refs/heads/master@{#579004} TBR=falken@chromium.org,kinuko@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 866769 , 854993 Change-Id: Ifacb8a30b851dc0d9808b26e01fecbd3dbbdb45d Reviewed-on: https://chromium-review.googlesource.com/1180901 Reviewed-by: Matt Falkenhagen <falken@chromium.org> Commit-Queue: Matt Falkenhagen <falken@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#584381}(cherry picked from commit 45fabee294b62e4e1c7c7cc36d6a779d80d4c57a) Reviewed-on: https://chromium-review.googlesource.com/1182881 Cr-Commit-Position: refs/branch-heads/3497@{#731} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/cbaf6152a96439c6846f07c14811e09315506367/content/browser/service_worker/service_worker_object_host.cc [modify] https://crrev.com/cbaf6152a96439c6846f07c14811e09315506367/content/browser/service_worker/service_worker_object_host.h [modify] https://crrev.com/cbaf6152a96439c6846f07c14811e09315506367/content/browser/service_worker/service_worker_provider_host.cc [modify] https://crrev.com/cbaf6152a96439c6846f07c14811e09315506367/content/browser/service_worker/service_worker_provider_host.h
,
Aug 21
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by bugdroid1@chromium.org
, Jul 24