New issue
Advanced search Search tips

Issue 866769 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 21
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Task



Sign in to add a comment

Remove instrumentation added for ServiceWorkerObjectHost related crash

Project Member Reported by falken@chromium.org, Jul 24

Issue description

Remove the instrumentation from issue 838410 and issue  854993 as they are now fixed.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bdff024cf35d3ca66c6f01c5dfaa38809d60a84a

commit bdff024cf35d3ca66c6f01c5dfaa38809d60a84a
Author: Matt Falkenhagen <falken@chromium.org>
Date: Tue Jul 24 05:02:15 2018

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}
[modify] https://crrev.com/bdff024cf35d3ca66c6f01c5dfaa38809d60a84a/content/browser/service_worker/service_worker_object_host.cc
[modify] https://crrev.com/bdff024cf35d3ca66c6f01c5dfaa38809d60a84a/content/browser/service_worker/service_worker_object_host.h
[modify] https://crrev.com/bdff024cf35d3ca66c6f01c5dfaa38809d60a84a/content/browser/service_worker/service_worker_provider_host.cc
[modify] https://crrev.com/bdff024cf35d3ca66c6f01c5dfaa38809d60a84a/content/browser/service_worker/service_worker_provider_host.h

Project Member

Comment 2 by bugdroid1@chromium.org, 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

Project Member

Comment 3 by bugdroid1@chromium.org, 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

Labels: -Pri-3 Merge-Request-69 OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows Pri-2
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.
Project Member

Comment 5 by sheriffbot@chromium.org, Aug 21

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
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
Is this M69 regression? If yes, how safe and critical is to merge cl listed at #3 to M69 this late in release cycle?
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.
Labels: -Merge-Review-69 Merge-Approved-69
Approving merge to M69 branch 3497 based on comments #4 and #7. Pls merge. Thank you.
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 21

Labels: -merge-approved-69 merge-merged-3497
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

Status: Fixed (was: Started)

Sign in to add a comment