Fix races when the same bits are downloaded from 2 URLs.

Fix races when the same bits are downloaded from 2 URLs.

Until now, we were saving downloaded file to the temporary directory
with a name being the hash of the file content. It means there is a race
when the same content is downloaded from 2 different URLs.

To fix this, we now create an intermediary directory that is the hash of
the URL.

Also, because this is only needed for debugging with gdb, and this is
inefficient in term of both CPU and storage (we do not know when to
delete the temporary directory), we control this with a command line
flag.

R=ncbray@chromium.org, eseidel@chromium.org
BUG=https://github.com/domokit/mojo/issues/61

Committed: https://chromium.googlesource.com/external/mojo/+/690a515b6e5f7a2224d7dbbdf74a8e2b6ceaf1c2

[Nick Bray (chromium), 2015-03-19 17:45:20]

Thanks for picking this up, I've been on a 2-day interview bender.

LGTM in light of time zone differences, but there also needs to be comments and
commit message info about the badness that happens when you have two mojo apps
sharing the same path on disk.  This is not a race, but a result of shared
library loading semantics that most people don't know or think about.  Comment
the crap out of this.  I do not want us forgetting it again.  You're fixing
multiple things but the commit message only mentions one.

https://codereview.chromium.org/1011333003/diff/1/shell/application_manager/n...
File shell/application_manager/network_fetcher.cc (right):

https://codereview.chromium.org/1011333003/diff/1/shell/application_manager/n...
shell/application_manager/network_fetcher.cc:131:
crypto::SHA256HashString(url.spec()).data(), crypto::kSHA256Length);
It's worth commenting this dirname will not be consistent for a particular
application because spec contains junk that is not considered part of a
canonical application name.  It's the leaf file name that needs to be consistent
for gdb serve. Comment / crosslink to a URL why we need this consistent naming,
too.

It also makes me wonder if there's a race where we have two sources trying to
connect to the same app at the same time.

https://codereview.chromium.org/1011333003/diff/1/shell/application_manager/n...
shell/application_manager/network_fetcher.cc:136: bool result =
base::CreateDirectoryAndGetError(app_dir, nullptr);
Comment on leaking the directory and why.

https://codereview.chromium.org/1011333003/diff/1/shell/switches.cc
File shell/switches.cc (right):

https://codereview.chromium.org/1011333003/diff/1/shell/switches.cc#newcode65
shell/switches.cc:65: // remote debugging.
Comment / crosslink / document.

[Aaron Boodman, 2015-03-20 00:29:55]

Why not just use the app's identity (e.g., sha1(app-url-minus-querystring)) as
the name to save under on disk?

[Nick Bray (chromium), 2015-03-20 02:45:07]

On 2015/03/20 00:29:55, Aaron Boodman wrote:
> Why not just use the app's identity (e.g., sha1(app-url-minus-querystring)) as
> the name to save under on disk?

That's more or less what I did in my version of this change.  The issue, as I
have come to understand, is when you're remote debugging and want to match a
bunch of files you have on disk to the files loaded on the remote machine. 
Knowing the app ID is harder than just hashing the contents of all the files you
have and matching the hashes.  In a perfect world there's a more elegant "build
ID" mechanism that GDB can use to match files in memory with files on disk, but
apparently the remote debugger is buggy and doesn't support this.

[qsr, 2015-03-20 10:19:47]

On 2015/03/19 17:45:20, Nick Bray (chromium) wrote:
> Thanks for picking this up, I've been on a 2-day interview bender.
> 
> LGTM in light of time zone differences, but there also needs to be comments
and
> commit message info about the badness that happens when you have two mojo apps
> sharing the same path on disk.  This is not a race, but a result of shared
> library loading semantics that most people don't know or think about.  Comment
> the crap out of this.  I do not want us forgetting it again.  You're fixing
> multiple things but the commit message only mentions one.

 I don't understand this point. We should never be loading two mojo apps from
the same path on disk (unless there is symbolic links, or hard links, and none
of this is not the problem of the shell, as it cannot even detect that
reliably). An application identity is the end URL without the query string. If
we have 2 URL that map to the same path on disk, they will be the same app, and
will only be loaded once.

 The issue was not that we were loading twice the same URL on disk, the problem
was that we were loading one app, while a second thread was writing the file.

> 
>
https://codereview.chromium.org/1011333003/diff/1/shell/application_manager/n...
> File shell/application_manager/network_fetcher.cc (right):
> 
>
https://codereview.chromium.org/1011333003/diff/1/shell/application_manager/n...
> shell/application_manager/network_fetcher.cc:131:
> crypto::SHA256HashString(url.spec()).data(), crypto::kSHA256Length);
> It's worth commenting this dirname will not be consistent for a particular
> application because spec contains junk that is not considered part of a
> canonical application name.  It's the leaf file name that needs to be
consistent
> for gdb serve. Comment / crosslink to a URL why we need this consistent
naming,
> too.
> 
> It also makes me wonder if there's a race where we have two sources trying to
> connect to the same app at the same time.

 I don't think there is. See ApplicationManager::HandleFetchCallback. When we
receive the content from the fetcher, the first thing we do is checking if the
app is not already started, and connecting to it if that's the case.

> 
>
https://codereview.chromium.org/1011333003/diff/1/shell/application_manager/n...
> shell/application_manager/network_fetcher.cc:136: bool result =
> base::CreateDirectoryAndGetError(app_dir, nullptr);
> Comment on leaking the directory and why.
> 
> https://codereview.chromium.org/1011333003/diff/1/shell/switches.cc
> File shell/switches.cc (right):
> 
> https://codereview.chromium.org/1011333003/diff/1/shell/switches.cc#newcode65
> shell/switches.cc:65: // remote debugging.
> Comment / crosslink / document.

[qsr, 2015-03-20 11:50:49]

https://codereview.chromium.org/1011333003/diff/1/shell/application_manager/n...
File shell/application_manager/network_fetcher.cc (right):

https://codereview.chromium.org/1011333003/diff/1/shell/application_manager/n...
shell/application_manager/network_fetcher.cc:131:
crypto::SHA256HashString(url.spec()).data(), crypto::kSHA256Length);
On 2015/03/19 17:45:20, Nick Bray (chromium) wrote:
> It's worth commenting this dirname will not be consistent for a particular
> application because spec contains junk that is not considered part of a
> canonical application name.  It's the leaf file name that needs to be
consistent
> for gdb serve. Comment / crosslink to a URL why we need this consistent
naming,
> too.

done

https://codereview.chromium.org/1011333003/diff/1/shell/application_manager/n...
shell/application_manager/network_fetcher.cc:136: bool result =
base::CreateDirectoryAndGetError(app_dir, nullptr);
On 2015/03/19 17:45:20, Nick Bray (chromium) wrote:
> Comment on leaking the directory and why.

Done.

https://codereview.chromium.org/1011333003/diff/1/shell/switches.cc
File shell/switches.cc (right):

https://codereview.chromium.org/1011333003/diff/1/shell/switches.cc#newcode65
shell/switches.cc:65: // remote debugging.
On 2015/03/19 17:45:20, Nick Bray (chromium) wrote:
> Comment / crosslink / document.

Done.

[qsr, 2015-03-20 11:51:06]

Committed patchset #3 (id:40001) manually as
690a515b6e5f7a2224d7dbbdf74a8e2b6ceaf1c2 (presubmit successful).

[Nick Bray (chromium), 2015-03-20 17:22:19]

On 2015/03/20 10:19:47, qsr wrote:
> On 2015/03/19 17:45:20, Nick Bray (chromium) wrote:
> > Thanks for picking this up, I've been on a 2-day interview bender.
> > 
> > LGTM in light of time zone differences, but there also needs to be comments
> and
> > commit message info about the badness that happens when you have two mojo
apps
> > sharing the same path on disk.  This is not a race, but a result of shared
> > library loading semantics that most people don't know or think about. 
Comment
> > the crap out of this.  I do not want us forgetting it again.  You're fixing
> > multiple things but the commit message only mentions one.
> 
>  I don't understand this point. We should never be loading two mojo apps from
> the same path on disk (unless there is symbolic links, or hard links, and none
> of this is not the problem of the shell, as it cannot even detect that
> reliably). An application identity is the end URL without the query string. If
> we have 2 URL that map to the same path on disk, they will be the same app,
and
> will only be loaded once.
> 
>  The issue was not that we were loading twice the same URL on disk, the
problem
> was that we were loading one app, while a second thread was writing the file.

Please re-read the commit message from the original CL:
https://codereview.chromium.org/1009003002/
I can believe racy writes to the temp directory were a problem, but the issue I
cite in that CL was the root problem for pretty much all the flake we were
seeing on the tree about a week ago.  I believe your change to stop DCHECKING on
MOJO_ABORT was treating a symptom of this problem.  That is where I started
investigating.
 
>  I don't think there is. See ApplicationManager::HandleFetchCallback. When we
> receive the content from the fetcher, the first thing we do is checking if the
> app is not already started, and connecting to it if that's the case.

Well, I think that almost works (but really should be commented more precisely
than "around we go again".)  However, because there can still be two fetches in
flight for the same application (de-duping does not occur until after the fetch
has happened), doesn't that still allow the same file truncation problem?  Just
for the same URL, rather than for different URLs?

[qsr, 2015-03-23 09:28:53]

On 2015/03/20 17:22:19, Nick Bray (chromium) wrote:
> On 2015/03/20 10:19:47, qsr wrote:
> > On 2015/03/19 17:45:20, Nick Bray (chromium) wrote:
> > > Thanks for picking this up, I've been on a 2-day interview bender.
> > > 
> > > LGTM in light of time zone differences, but there also needs to be
comments
> > and
> > > commit message info about the badness that happens when you have two mojo
> apps
> > > sharing the same path on disk.  This is not a race, but a result of shared
> > > library loading semantics that most people don't know or think about. 
> Comment
> > > the crap out of this.  I do not want us forgetting it again.  You're
fixing
> > > multiple things but the commit message only mentions one.
> > 
> >  I don't understand this point. We should never be loading two mojo apps
from
> > the same path on disk (unless there is symbolic links, or hard links, and
none
> > of this is not the problem of the shell, as it cannot even detect that
> > reliably). An application identity is the end URL without the query string.
If
> > we have 2 URL that map to the same path on disk, they will be the same app,
> and
> > will only be loaded once.
> > 
> >  The issue was not that we were loading twice the same URL on disk, the
> problem
> > was that we were loading one app, while a second thread was writing the
file.
> 
> Please re-read the commit message from the original CL:
> https://codereview.chromium.org/1009003002/
> I can believe racy writes to the temp directory were a problem, but the issue
I
> cite in that CL was the root problem for pretty much all the flake we were
> seeing on the tree about a week ago.  I believe your change to stop DCHECKING
on
> MOJO_ABORT was treating a symptom of this problem.  That is where I started
> investigating.

 Interesting, I know dlopen was looking very hard not to load the same library
twice (to the point that using hard link doesn't work), so I was expecting it to
look at the inode of the library, not at the place on disk... It seems it does
both...

>  
> >  I don't think there is. See ApplicationManager::HandleFetchCallback. When
we
> > receive the content from the fetcher, the first thing we do is checking if
the
> > app is not already started, and connecting to it if that's the case.
> 
> Well, I think that almost works (but really should be commented more precisely
> than "around we go again".)  However, because there can still be two fetches
in
> flight for the same application (de-duping does not occur until after the
fetch
> has happened), doesn't that still allow the same file truncation problem? 
Just
> for the same URL, rather than for different URLs?

 You are right. Will have to fix this. Thanks.