Use associated interface on DownloadedTempFile

Use associated interface on DownloadedTempFile

Blob systems depends on the ordering of loading IPC messages. That is,
the blob creation IPC via XHR is issued before the destruction of
DownloadedTempFile, and it expects to the blob IPC is handled before
DownloadedTempFile IPC.
However in the previous code, that is not guaranteed by Mojo, and that
makes Layout tests flaky. This CL switches DownLoadedTempFile in a loading
IPC to an associated interface to ensure it preserves the ordering of IPC
to other loading IPCs.

BUG=670562
Review-Url: https://codereview.chromium.org/2561743003
Cr-Commit-Position: refs/heads/master@{#445611}
Committed: https://chromium.googlesource.com/chromium/src/+/ad092abe5691a30391dbc0fd846af75e9917d37b

[tzik, 2016-12-08 08:12:12]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 08:12:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2016-12-08 08:12:34]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 08:12:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2016-12-08 08:41:48]

Description was changed from

==========
Use associated interface on DownloadedTempFile

BUG=
==========

to

==========
Use associated interface on DownloadedTempFile

BUG=670562
==========

[tzik, 2016-12-08 08:42:49]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 08:43:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2016-12-08 08:43:08]

tzik@chromium.org changed reviewers:
+ yhirano@chromium.org

[tzik, 2016-12-08 08:44:04]

PTAL

[commit-bot: I haz the power, 2016-12-08 10:36:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 10:36:29]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-12-09 03:49:23]

lgtm

https://codereview.chromium.org/2561743003/diff/40001/content/browser/loader/...
File content/browser/loader/downloaded_temp_file_impl.cc (right):

https://codereview.chromium.org/2561743003/diff/40001/content/browser/loader/...
content/browser/loader/downloaded_temp_file_impl.cc:8: #include
"mojo/public/cpp/bindings/strong_associated_binding.h"
+mojo/public/cpp/bindings/associated_group.h

https://codereview.chromium.org/2561743003/diff/40001/content/common/url_load...
File content/common/url_loader.mojom (right):

https://codereview.chromium.org/2561743003/diff/40001/content/common/url_load...
content/common/url_loader.mojom:38: OnReceiveResponse(URLResponseHead head,
associated DownloadedTempFile? downloaded_file);
reflow needed?

[tzik, 2016-12-09 07:06:56]

Description was changed from

==========
Use associated interface on DownloadedTempFile

BUG=670562
==========

to

==========
Use associated interface on DownloadedTempFile

Blob systems depends on the ordering of loading IPC messages. That is,
the blob creation IPC via XHR is issued before the destruction of
DownloadedTempFile, and it expects to the blob IPC is handled before
DownloadedTempFile IPC.
However in the previous code, that is not guaranteed by Mojo, and that
makes Layout tests flaky. This CL switches DownLoadedTempFile in a loading
IPC to an associated interface to ensure it preserves the ordering of IPC
to other loading IPCs.

BUG=670562
==========

[tzik, 2016-12-09 07:06:56]

tzik@chromium.org changed reviewers:
+ mmenke@chromium.org

[tzik, 2016-12-09 07:07:25]

tzik@chromium.org changed reviewers:
+ jam@chromium.org

[tzik, 2016-12-09 07:07:46]

tzik@chromium.org changed reviewers:
+ dcheng@chromium.org

[tzik, 2016-12-09 07:08:39]

+mmenke, jam and dcheng for OWNER review.
PTAL to:
mmenke: overall
jam: //content
dcheng: *.mojom

https://codereview.chromium.org/2561743003/diff/40001/content/browser/loader/...
File content/browser/loader/downloaded_temp_file_impl.cc (right):

https://codereview.chromium.org/2561743003/diff/40001/content/browser/loader/...
content/browser/loader/downloaded_temp_file_impl.cc:8: #include
"mojo/public/cpp/bindings/strong_associated_binding.h"
On 2016/12/09 03:49:23, yhirano wrote:
> +mojo/public/cpp/bindings/associated_group.h

Done.

https://codereview.chromium.org/2561743003/diff/40001/content/common/url_load...
File content/common/url_loader.mojom (right):

https://codereview.chromium.org/2561743003/diff/40001/content/common/url_load...
content/common/url_loader.mojom:38: OnReceiveResponse(URLResponseHead head,
associated DownloadedTempFile? downloaded_file);
On 2016/12/09 03:49:23, yhirano wrote:
> reflow needed?

Done.

[tzik, 2016-12-09 07:09:36]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 07:09:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 09:01:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 09:01:20]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-12-12 09:09:55]

mojo lgtm

[mmenke, 2016-12-13 20:24:06]

https://codereview.chromium.org/2561743003/diff/60001/content/browser/loader/...
File content/browser/loader/downloaded_temp_file_impl.cc (right):

https://codereview.chromium.org/2561743003/diff/60001/content/browser/loader/...
content/browser/loader/downloaded_temp_file_impl.cc:20:
mojo::AssociatedInterfaceRequest<mojom::DownloadedTempFile> request;
Are there any docs on what an associated interface is?  All mojo class-level
docs I see assume you already know that.

https://codereview.chromium.org/2561743003/diff/60001/content/browser/loader/...
File content/browser/loader/downloaded_temp_file_impl.h (right):

https://codereview.chromium.org/2561743003/diff/60001/content/browser/loader/...
content/browser/loader/downloaded_temp_file_impl.h:27: int request_id);
Please put a line break between these two methods.  The two-lines for the
declaration of the first one makes this hard to read.

[dcheng, 2016-12-13 22:37:44]

https://codereview.chromium.org/2561743003/diff/60001/content/browser/loader/...
File content/browser/loader/downloaded_temp_file_impl.cc (right):

https://codereview.chromium.org/2561743003/diff/60001/content/browser/loader/...
content/browser/loader/downloaded_temp_file_impl.cc:20:
mojo::AssociatedInterfaceRequest<mojom::DownloadedTempFile> request;
On 2016/12/13 20:24:06, mmenke wrote:
> Are there any docs on what an associated interface is?  All mojo class-level
> docs I see assume you already know that.

I guess we should clarify the docs.

Basically, with Mojo, the default is each interface you connect to has its own
message pipe. Since there is no ordering guarantee between two message pipes,
that means it's possible for messages on different pipes to race.

With IPC:
  sender->Send(new FooMsg(...));
  sender->Send(new BarMsg(...));

is well-ordered: it's guaranteed the receiver will process FooMsg before BarMsg.

However, with Mojo:
  foo->This(...);
  bar->That(...);

Either ordering is possible: the IPCs are sent on different message pipes, and
there's no guarantee which one will be handled first by the receiver.

Associated interfaces are designed to solve this problem by allowing multiple
mojo interface instances to share a single message pipe. If |foo| and |bar| are
associated interfaces, then it's guaranteed that This(...) will be handled
before That(...).

[tzik, 2016-12-14 07:08:08]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 07:08:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2016-12-14 07:08:29]

https://codereview.chromium.org/2561743003/diff/60001/content/browser/loader/...
File content/browser/loader/downloaded_temp_file_impl.h (right):

https://codereview.chromium.org/2561743003/diff/60001/content/browser/loader/...
content/browser/loader/downloaded_temp_file_impl.h:27: int request_id);
On 2016/12/13 20:24:06, mmenke wrote:
> Please put a line break between these two methods.  The two-lines for the
> declaration of the first one makes this hard to read.

Done.

[commit-bot: I haz the power, 2016-12-14 08:51:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 08:51:06]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-12-14 23:19:15]

On 2016/12/14 07:08:29, tzik wrote:
>
https://codereview.chromium.org/2561743003/diff/60001/content/browser/loader/...
> File content/browser/loader/downloaded_temp_file_impl.h (right):
> 
>
https://codereview.chromium.org/2561743003/diff/60001/content/browser/loader/...
> content/browser/loader/downloaded_temp_file_impl.h:27: int request_id);
> On 2016/12/13 20:24:06, mmenke wrote:
> > Please put a line break between these two methods.  The two-lines for the
> > declaration of the first one makes this hard to read.
> 
> Done.

Forgot to get to this today - first thing in my queue tomorrow.

[mmenke, 2016-12-15 16:48:19]

https://codereview.chromium.org/2561743003/diff/80001/content/browser/loader/...
File content/browser/loader/downloaded_temp_file_impl.cc (right):

https://codereview.chromium.org/2561743003/diff/80001/content/browser/loader/...
content/browser/loader/downloaded_temp_file_impl.cc:25: std::move(request));
Sorry for all the round trips, but I really have no clue what this does.  We
have a "associated_group" (Which I guess is a group of things that share a pipe)
from the ResourceHandler, and want to make a mojo object using that pipe on the
shared interface.

So the we create the interface on the pipe...But then what is a PtrInfo?  What
is an interface request?  What does making a strong binding not return anything?

I'm having trouble mapping this code to what it's actually doing.  The
method/class descriptions don't provide much help.  I'm pushing back on this
largely because I find this word soup a common problem with mojo setup, and this
seems a much more confusing soup than usual.

https://codereview.chromium.org/2561743003/diff/80001/content/browser/loader/...
File content/browser/loader/downloaded_temp_file_impl.h (right):

https://codereview.chromium.org/2561743003/diff/80001/content/browser/loader/...
content/browser/loader/downloaded_temp_file_impl.h:28: //    client. It keeps
alive until the client destroyes the other endpoint.
nit: destroys

[mmenke, 2016-12-15 16:51:08]

https://codereview.chromium.org/2561743003/diff/80001/content/browser/loader/...
File content/browser/loader/downloaded_temp_file_impl.cc (right):

https://codereview.chromium.org/2561743003/diff/80001/content/browser/loader/...
content/browser/loader/downloaded_temp_file_impl.cc:25: std::move(request));
On 2016/12/15 16:48:19, mmenke (Out Dec 17 to Jan 2) wrote:
> Sorry for all the round trips, but I really have no clue what this does.  We
> have a "associated_group" (Which I guess is a group of things that share a
pipe)
> from the ResourceHandler, and want to make a mojo object using that pipe on
the
> shared interface.
> 
> So the we create the interface on the pipe...But then what is a PtrInfo?  What
> is an interface request?  What does making a strong binding not return
anything?
> 
> I'm having trouble mapping this code to what it's actually doing.  The
> method/class descriptions don't provide much help.  I'm pushing back on this
> largely because I find this word soup a common problem with mojo setup, and
this
> seems a much more confusing soup than usual.

And if we want Mojo to be the way for everyone to do things, including people
not directly involved in mojo development, I think code this confusing really
needs to either be better explained, renamed, or otherwise cleaned up to be more
intuitive

[tzik, 2017-01-05 05:13:12]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 05:13:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 06:54:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 06:54:25]

Dry run: This issue passed the CQ dry run.

[tzik, 2017-01-05 09:43:31]

tzik@chromium.org changed reviewers:
+ yzshen@chromium.org

[tzik, 2017-01-05 10:47:56]

+yzshen, as a mojo expert. Any opinion about #31 & #32?

https://codereview.chromium.org/2561743003/diff/80001/content/browser/loader/...
File content/browser/loader/downloaded_temp_file_impl.cc (right):

https://codereview.chromium.org/2561743003/diff/80001/content/browser/loader/...
content/browser/loader/downloaded_temp_file_impl.cc:25: std::move(request));
On 2016/12/15 16:48:19, mmenke wrote:
> Sorry for all the round trips, but I really have no clue what this does.  We
> have a "associated_group" (Which I guess is a group of things that share a
pipe)
> from the ResourceHandler, and want to make a mojo object using that pipe on
the
> shared interface.
> 
> So the we create the interface on the pipe...But then what is a PtrInfo?

Hmm, I don't have good explanation that.
Yuzhu: Could you help us to understand the difference of AssociatedInterfacePtr
and AssociatedInterfacePtr*Info*? Why do we need to use PtrInfo instead of Ptr
on "associated" keyword in .mojom?

> What is an interface request?  What does making a strong binding not return
anything?

Since we don't have suitable helper function for the instance set up, I'm using
the interface request as the raw set up sequence.
I think we should add a helper to hide that as a part of implementation detail.

> 
> I'm having trouble mapping this code to what it's actually doing.  The
> method/class descriptions don't provide much help.  I'm pushing back on this
> largely because I find this word soup a common problem with mojo setup, and
this
> seems a much more confusing soup than usual.

[yzshen1, 2017-01-13 15:55:03]

Sorry, I wrote a reply a week ago but forgot to publish it. :/

https://codereview.chromium.org/2561743003/diff/80001/content/browser/loader/...
File content/browser/loader/downloaded_temp_file_impl.cc (right):

https://codereview.chromium.org/2561743003/diff/80001/content/browser/loader/...
content/browser/loader/downloaded_temp_file_impl.cc:25: std::move(request));
On 2017/01/05 10:47:56, tzik wrote:
> On 2016/12/15 16:48:19, mmenke wrote:
> > Sorry for all the round trips, but I really have no clue what this does.  We
> > have a "associated_group" (Which I guess is a group of things that share a
> pipe)
> > from the ResourceHandler, and want to make a mojo object using that pipe on
> the
> > shared interface.
> > 
> > So the we create the interface on the pipe...But then what is a PtrInfo?
> 
> Hmm, I don't have good explanation that.
> Yuzhu: Could you help us to understand the difference of
AssociatedInterfacePtr
> and AssociatedInterfacePtr*Info*? Why do we need to use PtrInfo instead of Ptr
> on "associated" keyword in .mojom?

AssociatedInterfacePtrInfo is the "handle" that could be used to construct an
AssociatedInterfacePtr.

AssociatedInterfacePtr is the proxy object that you can set up to make method
calls. It is sending request and monitoring response using that "handle".

The reason why we map a mojom associated interface parameter to AIPtrInfo
instead of AIPtr: because it doesn't make sense to construct an AIPtr when you
want to pass this associated interface to the remote side.


> 
> > What is an interface request? 

I think it is unavoidable that mojo users have to learn about the mojo
terminology. :) 

There are documents about Mojo here:
https://www.chromium.org/developers/design-documents/mojo

In particular, we have document about associated interfaces here:
https://www.chromium.org/developers/design-documents/mojo/associated-interfaces



> What does making a strong binding not return
> anything?
> 
> Since we don't have suitable helper function for the instance set up, I'm
using
> the interface request as the raw set up sequence.
> I think we should add a helper to hide that as a part of implementation
detail.
> 
> > 
> > I'm having trouble mapping this code to what it's actually doing.  The
> > method/class descriptions don't provide much help.  I'm pushing back on this
> > largely because I find this word soup a common problem with mojo setup, and
> this
> > seems a much more confusing soup than usual.
>

[yzshen1, 2017-01-13 17:45:44]

(I will have a more detailed reply in the corresponding mail thread.)

[mmenke, 2017-01-13 17:48:33]

https://codereview.chromium.org/2561743003/diff/80001/content/browser/loader/...
File content/browser/loader/downloaded_temp_file_impl.cc (right):

https://codereview.chromium.org/2561743003/diff/80001/content/browser/loader/...
content/browser/loader/downloaded_temp_file_impl.cc:25: std::move(request));
On 2017/01/13 15:55:03, yzshen1 wrote:
> On 2017/01/05 10:47:56, tzik wrote:
> > On 2016/12/15 16:48:19, mmenke wrote:
> > > Sorry for all the round trips, but I really have no clue what this does. 
We
> > > have a "associated_group" (Which I guess is a group of things that share a
> > pipe)
> > > from the ResourceHandler, and want to make a mojo object using that pipe
on
> > the
> > > shared interface.
> > > 
> > > So the we create the interface on the pipe...But then what is a PtrInfo?
> > 
> > Hmm, I don't have good explanation that.
> > Yuzhu: Could you help us to understand the difference of
> AssociatedInterfacePtr
> > and AssociatedInterfacePtr*Info*? Why do we need to use PtrInfo instead of
Ptr
> > on "associated" keyword in .mojom?
> 
> AssociatedInterfacePtrInfo is the "handle" that could be used to construct an
> AssociatedInterfacePtr.
> 
> AssociatedInterfacePtr is the proxy object that you can set up to make method
> calls. It is sending request and monitoring response using that "handle".
> 
> The reason why we map a mojom associated interface parameter to AIPtrInfo
> instead of AIPtr: because it doesn't make sense to construct an AIPtr when you
> want to pass this associated interface to the remote side.
> 
> 
> > 
> > > What is an interface request? 
> 
> I think it is unavoidable that mojo users have to learn about the mojo
> terminology. :) 

I agree with this, which is why I was shocked to find all these classes and
objects undocumented or poorly documented in the code.

> There are documents about Mojo here:
> https://www.chromium.org/developers/design-documents/mojo
> 
> In particular, we have document about associated interfaces here:
>
https://www.chromium.org/developers/design-documents/mojo/associated-interfaces
> 
> 
> 
> > What does making a strong binding not return
> > anything?
> > 
> > Since we don't have suitable helper function for the instance set up, I'm
> using
> > the interface request as the raw set up sequence.
> > I think we should add a helper to hide that as a part of implementation
> detail.
> > 
> > > 
> > > I'm having trouble mapping this code to what it's actually doing.  The
> > > method/class descriptions don't provide much help.  I'm pushing back on
this
> > > largely because I find this word soup a common problem with mojo setup,
and
> > this
> > > seems a much more confusing soup than usual.
> > 
>

[mmenke, 2017-01-13 20:59:57]

Sorry, haven't had a chance to follow up on the Mojo discussion today.  Ball is
in my court on this.

[mmenke, 2017-01-18 20:06:43]

On 2017/01/13 20:59:57, mmenke wrote:
> Sorry, haven't had a chance to follow up on the Mojo discussion today.  Ball
is
> in my court on this.

LGTM, don't want to block you on this any more.  May spend some time suggesting
beefing up the inline mojo docs when I have time.

[mmenke, 2017-01-18 20:08:53]

https://codereview.chromium.org/2561743003/diff/120001/content/browser/loader...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2561743003/diff/120001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler.cc:175: if
(!response->head.download_file_path.empty()) {
Hrm....I assume we never do this for main frames (Or sub frames)?  We'll want to
be passing main frame URLLoaders from the renderer to the browser in response to
OnResponseStarted calls, which we can't do once we have associated interfaces, I
believe?

[tzik, 2017-01-19 05:42:08]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 05:42:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 05:44:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 05:44:30]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[tzik, 2017-01-19 11:01:06]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 11:01:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 12:15:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 12:15:04]

Dry run: This issue passed the CQ dry run.

[tzik, 2017-01-19 13:03:27]

tzik@chromium.org changed reviewers:
+ kinuko@chromium.org
- jam@chromium.org

[tzik, 2017-01-19 13:04:41]

+kinuko-san for //content owner review. PTAL.

https://codereview.chromium.org/2561743003/diff/120001/content/browser/loader...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2561743003/diff/120001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler.cc:175: if
(!response->head.download_file_path.empty()) {
On 2017/01/18 20:08:53, mmenke wrote:
> Hrm....I assume we never do this for main frames (Or sub frames)?  We'll want
to
> be passing main frame URLLoaders from the renderer to the browser in response
to
> OnResponseStarted calls, which we can't do once we have associated interfaces,
I
> believe?

Do you mean TransferredNavigation case? Right, it's only for the navigation and
a request for the main resource, for which we don't use download-to-file mode.

[kinuko, 2017-01-19 13:08:04]

lgtm

[mmenke, 2017-01-19 13:09:08]

https://codereview.chromium.org/2561743003/diff/120001/content/browser/loader...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2561743003/diff/120001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler.cc:175: if
(!response->head.download_file_path.empty()) {
On 2017/01/19 13:04:41, tzik wrote:
> On 2017/01/18 20:08:53, mmenke wrote:
> > Hrm....I assume we never do this for main frames (Or sub frames)?  We'll
want
> to
> > be passing main frame URLLoaders from the renderer to the browser in
response
> to
> > OnResponseStarted calls, which we can't do once we have associated
interfaces,
> I
> > believe?
> 
> Do you mean TransferredNavigation case? Right, it's only for the navigation
and
> a request for the main resource, for which we don't use download-to-file mode.

I mean cases in which we'd want to transfer the URLLoader around.  For
navigations (Both main frame, with PlzNavigate, and subframes, with
PlzNavigate+OOPIF), we'd want to drive the URLLoader from the navigation code,
and then pass it to the renderer (Or download subsystem, if we discover it's a
download)

[tzik, 2017-01-23 12:12:49]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 12:13:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 14:33:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 14:33:58]

Dry run: This issue passed the CQ dry run.

[tzik, 2017-01-23 19:21:34]

On 2017/01/19 13:09:08, mmenke wrote:
>
https://codereview.chromium.org/2561743003/diff/120001/content/browser/loader...
> File content/browser/loader/mojo_async_resource_handler.cc (right):
> 
>
https://codereview.chromium.org/2561743003/diff/120001/content/browser/loader...
> content/browser/loader/mojo_async_resource_handler.cc:175: if
> (!response->head.download_file_path.empty()) {
> On 2017/01/19 13:04:41, tzik wrote:
> > On 2017/01/18 20:08:53, mmenke wrote:
> > > Hrm....I assume we never do this for main frames (Or sub frames)?  We'll
> want
> > to
> > > be passing main frame URLLoaders from the renderer to the browser in
> response
> > to
> > > OnResponseStarted calls, which we can't do once we have associated
> interfaces,
> > I
> > > believe?
> > 
> > Do you mean TransferredNavigation case? Right, it's only for the navigation
> and
> > a request for the main resource, for which we don't use download-to-file
mode.
> 
> I mean cases in which we'd want to transfer the URLLoader around.  For
> navigations (Both main frame, with PlzNavigate, and subframes, with
> PlzNavigate+OOPIF), we'd want to drive the URLLoader from the navigation code,
> and then pass it to the renderer (Or download subsystem, if we discover it's a
> download)

Since we don't transfer URLLoader in download-to-file mode, can we just leave a
DCHECK for now?

[mmenke, 2017-01-23 19:32:15]

On 2017/01/23 19:21:34, tzik wrote:
> On 2017/01/19 13:09:08, mmenke wrote:
> >
>
https://codereview.chromium.org/2561743003/diff/120001/content/browser/loader...
> > File content/browser/loader/mojo_async_resource_handler.cc (right):
> > 
> >
>
https://codereview.chromium.org/2561743003/diff/120001/content/browser/loader...
> > content/browser/loader/mojo_async_resource_handler.cc:175: if
> > (!response->head.download_file_path.empty()) {
> > On 2017/01/19 13:04:41, tzik wrote:
> > > On 2017/01/18 20:08:53, mmenke wrote:
> > > > Hrm....I assume we never do this for main frames (Or sub frames)?  We'll
> > want
> > > to
> > > > be passing main frame URLLoaders from the renderer to the browser in
> > response
> > > to
> > > > OnResponseStarted calls, which we can't do once we have associated
> > interfaces,
> > > I
> > > > believe?
> > > 
> > > Do you mean TransferredNavigation case? Right, it's only for the
navigation
> > and
> > > a request for the main resource, for which we don't use download-to-file
> mode.
> > 
> > I mean cases in which we'd want to transfer the URLLoader around.  For
> > navigations (Both main frame, with PlzNavigate, and subframes, with
> > PlzNavigate+OOPIF), we'd want to drive the URLLoader from the navigation
code,
> > and then pass it to the renderer (Or download subsystem, if we discover it's
a
> > download)
> 
> Since we don't transfer URLLoader in download-to-file mode, can we just leave
a
> DCHECK for now?

That's what I was thinking.

[tzik, 2017-01-24 02:24:20]

The CQ bit was checked by tzik@chromium.org

[tzik, 2017-01-24 02:24:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
yhirano@chromium.org, dcheng@chromium.org, mmenke@chromium.org,
kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2561743003/#ps180001
(title: "+NOTREACHED on non frame trasfer")

[commit-bot: I haz the power, 2017-01-24 02:25:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 02:34:29]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1485224660465320,
"parent_rev": "2f08dd5ad90c907e1575be2e67ae7b18fc062ef0", "commit_rev":
"ad092abe5691a30391dbc0fd846af75e9917d37b"}

[commit-bot: I haz the power, 2017-01-24 02:35:06]

Description was changed from

==========
Use associated interface on DownloadedTempFile

Blob systems depends on the ordering of loading IPC messages. That is,
the blob creation IPC via XHR is issued before the destruction of
DownloadedTempFile, and it expects to the blob IPC is handled before
DownloadedTempFile IPC.
However in the previous code, that is not guaranteed by Mojo, and that
makes Layout tests flaky. This CL switches DownLoadedTempFile in a loading
IPC to an associated interface to ensure it preserves the ordering of IPC
to other loading IPCs.

BUG=670562
==========

to

==========
Use associated interface on DownloadedTempFile

Blob systems depends on the ordering of loading IPC messages. That is,
the blob creation IPC via XHR is issued before the destruction of
DownloadedTempFile, and it expects to the blob IPC is handled before
DownloadedTempFile IPC.
However in the previous code, that is not guaranteed by Mojo, and that
makes Layout tests flaky. This CL switches DownLoadedTempFile in a loading
IPC to an associated interface to ensure it preserves the ordering of IPC
to other loading IPCs.

BUG=670562

Review-Url: https://codereview.chromium.org/2561743003
Cr-Commit-Position: refs/heads/master@{#445611}
Committed:
https://chromium.googlesource.com/chromium/src/+/ad092abe5691a30391dbc0fd846a...
==========

[commit-bot: I haz the power, 2017-01-24 02:35:08]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/ad092abe5691a30391dbc0fd846a...

[mastiz, 2017-01-25 09:46:24]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2658583002/ by mastiz@chromium.org.

The reason for reverting is: Culprit suspect for failing ThreadSanitizer tests,
e.g.
https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20TSan%....