New issue
Advanced search Search tips

Issue 911652 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 3
Type: Bug



Sign in to add a comment

Check for data pipe creation failure in AboutURLLoader.

Project Member Reported by arthurso...@chromium.org, Dec 4

Issue description

There are no check about pipe creation failure in AboutURLLoader.

When resources are not sufficient. It will crash.
For the moment, it never happened because this URLLoader is almost not used, but it can happen.

This regression has been introduced in M72. It would be nice to cherry-pick the fix into M72.
https://chromium-review.googlesource.com/c/chromium/src/+/1355124
 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 4

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

commit 7d47a55b52ba95cb87b5f71087762f8eae06e6d9
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Tue Dec 04 15:47:38 2018

AboutURLLoader: check for data pipe creation.

The class mojo::DataPipe shouldn't be used. In its constructor, it
CHECKs the data pipe is created. It can fails when system resources are
insufficient.

This CL replaces the mojo::DataPipe and handles the insuficient resource
case.

Note: No crash reports has been received so far because this URLLoader
      is not used often enough.

Bug:  911652 
Change-Id: I09e9ec7cf883a3682466e528b6010fe326916400
Reviewed-on: https://chromium-review.googlesource.com/c/1355124
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613562}
[modify] https://crrev.com/7d47a55b52ba95cb87b5f71087762f8eae06e6d9/content/browser/loader/navigation_url_loader_impl.cc

Labels: M-72 Merge-Request-72
Please see comment 1.
I am requesting merging this patch. This patch is simple and well understood. It will avoid crashes.

It is almost the same as the fix in bug 905779. The latter reached M72 just before branch cut.
Labels: -Merge-Request-72 Merge-Approved-72
Pls merge your change to M72 branch 3626 ASAP so we can pick it up for tomororw's dev release. Thank you.
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 5

Labels: -merge-approved-72 merge-merged-3626
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/964eb462f0ea0197c23511bb1976b7cfd4eaa065

commit 964eb462f0ea0197c23511bb1976b7cfd4eaa065
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Wed Dec 05 08:47:13 2018

AboutURLLoader: check for data pipe creation.

The class mojo::DataPipe shouldn't be used. In its constructor, it
CHECKs the data pipe is created. It can fails when system resources are
insufficient.

This CL replaces the mojo::DataPipe and handles the insuficient resource
case.

Note: No crash reports has been received so far because this URLLoader
      is not used often enough.

Bug:  911652 
Change-Id: I09e9ec7cf883a3682466e528b6010fe326916400
Reviewed-on: https://chromium-review.googlesource.com/c/1355124
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#613562}(cherry picked from commit 7d47a55b52ba95cb87b5f71087762f8eae06e6d9)
Reviewed-on: https://chromium-review.googlesource.com/c/1361873
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#68}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/964eb462f0ea0197c23511bb1976b7cfd4eaa065/content/browser/loader/navigation_url_loader_impl.cc

Status: Fixed (was: Started)
Labels: CommitLog-Audit-Violation Merge-Without-Approval
Here's a summary of the rules that were executed: 
 - OnlyMergeApprovedChange: Rule Failed -- Revision 964eb462f0ea0197c23511bb1976b7cfd4eaa065 was merged to refs/branch-heads/3626 branch with no merge approval from a TPM! 
Please explain why this change was merged to the branch!
 - AcknowledgeMerge: Notification Required -- 
Labels: Merge-Merged-72-3626
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/964eb462f0ea0197c23511bb1976b7cfd4eaa065

Commit: 964eb462f0ea0197c23511bb1976b7cfd4eaa065
Author: arthursonzogni@chromium.org
Commiter: arthursonzogni@chromium.org
Date: 2018-12-05 08:47:13 +0000 UTC

AboutURLLoader: check for data pipe creation.

The class mojo::DataPipe shouldn't be used. In its constructor, it
CHECKs the data pipe is created. It can fails when system resources are
insufficient.

This CL replaces the mojo::DataPipe and handles the insuficient resource
case.

Note: No crash reports has been received so far because this URLLoader
      is not used often enough.

Bug:  911652 
Change-Id: I09e9ec7cf883a3682466e528b6010fe326916400
Reviewed-on: https://chromium-review.googlesource.com/c/1355124
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#613562}(cherry picked from commit 7d47a55b52ba95cb87b5f71087762f8eae06e6d9)
Reviewed-on: https://chromium-review.googlesource.com/c/1361873
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#68}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
Labels: -CommitLog-Audit-Violation -Merge-Without-Approval
This merge was approved at #3 by djmm@ (Chrome OS M72 TPM).

Sign in to add a comment