New issue
Advanced search Search tips

Issue 776310 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

LorgnetteManagerClientImpl::ScanImageToString has race.

Project Member Reported by hidehiko@chromium.org, Oct 19 2017

Issue description

If ScanImageToString is called,
it sends a D-Bus Call to LorgnetteManager service, and in parallel,
it starts to read the data from pipe, whose write side is sent to the service via the D-Bus call.

Periodically, PipeReaderForString's OnDataReady() callback is called,
per buffer full.
On completion, the callback for D-Bus is called.

There's no guarantee of the order, so D-Bus's callback can be called before pending PipeReaderForString::OnDataReady().
In such a case, OnDataReady(-1) is manually called, though it means pending data in the buffer will be simply discarded.

It is necessary to wait until the data is processed, at least on success case.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 30 2017

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

commit 1ac7aa2868737f50904f62041a58f23308c58ea1
Author: Hidehiko Abe <hidehiko@chromium.org>
Date: Mon Oct 30 20:35:19 2017

Refactor chromeos::PipeReader.

Before this change, PipeReader cannot report an error.
With this change, it can.
Also, simplify the cancel flow. Now, as a common pattern
used across chrome repository, just deleting the instance
will cancel the inflight operation.

Also, PipeReaderForString was the only subclass. So merged
PipeReader.

Added unittests.

BUG= 776310 
TEST=Ran bots.

Change-Id: I6cda997b36db1703f79f9fe5e682f95af923ff3d
Reviewed-on: https://chromium-review.googlesource.com/727445
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Simon Que <sque@chromium.org>
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512607}
[modify] https://crrev.com/1ac7aa2868737f50904f62041a58f23308c58ea1/chrome/browser/metrics/perf/perf_output.cc
[modify] https://crrev.com/1ac7aa2868737f50904f62041a58f23308c58ea1/chrome/browser/metrics/perf/perf_output.h
[modify] https://crrev.com/1ac7aa2868737f50904f62041a58f23308c58ea1/chromeos/BUILD.gn
[modify] https://crrev.com/1ac7aa2868737f50904f62041a58f23308c58ea1/chromeos/dbus/debug_daemon_client.cc
[modify] https://crrev.com/1ac7aa2868737f50904f62041a58f23308c58ea1/chromeos/dbus/lorgnette_manager_client.cc
[modify] https://crrev.com/1ac7aa2868737f50904f62041a58f23308c58ea1/chromeos/dbus/pipe_reader.cc
[modify] https://crrev.com/1ac7aa2868737f50904f62041a58f23308c58ea1/chromeos/dbus/pipe_reader.h
[add] https://crrev.com/1ac7aa2868737f50904f62041a58f23308c58ea1/chromeos/dbus/pipe_reader_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Nov 10 2017

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

commit 870cb9538a6929f4ca45ae6582be21638b26452f
Author: Hidehiko Abe <hidehiko@chromium.org>
Date: Fri Nov 10 02:07:22 2017

Fix race issue in LorgnetteManagerClientImpl.

There was no guarantee between data read completion on
a blocking thread and the D-Bus reply.
If D-Bus reply is earlier than the data read,
the data would be lost.

This CL fixes it by waiting both data read completion and
D-Bus reply.

BUG= 776310 
TEST=Ran documentscan_AppTestWithFakeLorgnette on DUT.

Change-Id: If887acf77b9451e52021d06d27f90852f961285f
Reviewed-on: https://chromium-review.googlesource.com/754149
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515418}
[modify] https://crrev.com/870cb9538a6929f4ca45ae6582be21638b26452f/chromeos/dbus/lorgnette_manager_client.cc

Status: Fixed (was: Started)

Comment 4 by dchan@chromium.org, Jan 22 2018

Status: archived (was: Fixed)

Comment 5 by dchan@chromium.org, Jan 23 2018

Status: Fixed (was: Archived)

Sign in to add a comment