New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 150861 link

Starred by 8 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 0
Type: Bug-Regression



Sign in to add a comment

Filesystem: URLs no longer work in platform apps

Project Member Reported by dbeam@chromium.org, Sep 19 2012

Issue description

Version: 23.0.1270.0
OS: Linux

What steps will reproduce the problem?
1. Save something to the HTML5 file system
2. Try to get it out with a filesystem: URL

What is the expected output? What do you see instead?
I expect this to work, instead I'm given many many console errors with:

  GET filesystem:chrome-extension://blah/persistent/blah/blah

When I inspect the file locally in the extension's directory in my profile it's fine.

Please use labels and text to provide additional information.
Bisected to ajwong@'s r157284 (http://src.chromium.org/viewvc/chrome?view=rev&revision=157284); reverting locally fixes the issues.
 

Comment 1 by dbeam@chromium.org, Sep 19 2012

Cc: creis@chromium.org nasko@chromium.org dbeam@chromium.org rahulrc@chromium.org
 Issue 150865  has been merged into this issue.
Labels: -Pri-1 -Feature-FileSystem Pri-0 Mstone-23
Summary: Filesystem: URLs no longer work in platform apps
I believe Albert is out today?  Can we wait till he's back - I believe this will will qualify as merge-worthy for M23 if it's something Albert can fix.

Alternative is a global revert?

Comment 4 by creis@chromium.org, Sep 19 2012

Albert's out sick today, but that sounds like an issue with his CL (which is needed for Apps V2 in M23).

dbeam@, are you trying to access this URL from within a V2 app?  Is it the same app as the chrome-extension://blah origin in the filesystem URL?  If so, we should be able to make that work.  I can look soon if Albert isn't able to respond.

Comment 5 by creis@chromium.org, Sep 19 2012

rahulrc@: I agree that it would be best to merge a small fix rather than revert the larger CL if possible.  It'll be much harder to unrevert the large CL on the M23 branch, and that's needed for Apps V2 storage isolation.  We'll try to get it resolved as quickly as possible.

Comment 6 by dbeam@chromium.org, Sep 19 2012

#4: creis@ yes, accessing files created from and living on the same origin. it worked fine until this CL.

Comment 7 by ajwong@chromium.org, Sep 19 2012

I came in.  Coffee cures all colds (I hope). :-/

Rahul: Reverting is a last-resort option because it would force a fairly large data migration of user profiles when browser tag launches.  That's why we spent the last month rushing isolation changes for you guys in the first place.

Let me see if I can track down why this is happening.

Comment 8 by ajwong@chromium.org, Sep 20 2012

Cc: jhawkins@chromium.org michaeln@chromium.org kinuko@chromium.org ericu@chromium.org
Status: Started
Okay, I think I tracked down what is happening.  Only 80% confidence at this point, but here's the notes.

(1) Only PERSISTENT files fail.  window.TEMPORARY ones seem to work. This is why the PlatformApps's iframe test (which exercises the FileSystem API) doesn't fail; thye only use temporary files.

(2) The file is created okay. It's the resolution of the url back into the file entry that is failing.

I think what's going on is ResourceDispatcherHost is using the wrong URLRequestContext to service the filesytem:// url.  Unfortunately, I haven't been able to track down where the request is actually passed off onto a URLRequestContext.  I'm betting the current code gets the URLRequestContext out of ResourceContext.

Eric, Michael, Kinuko, I'm wondering if any of you might know more about how this code path works.  I'll keep looking but one of you happen to know off the opt of your head how the piping here works, that'd be great.

Comment 9 by tzik@chromium.org, Sep 20 2012

Cc: tzik@chromium.org
By code inspection, I'm thinking the culprit is  ResourceDispatcherHostImpl::HandleExternalProtocol().  This line looks suspect:

  const net::URLRequestJobFactory* job_factory =
      info->GetContext()->GetRequestContext()->job_factory();

the info->GetContext() will return the "default" request context.  Assuming HandleExternalProtocol() is triggered for filesystem: URLs, this will get the incorrect URLRequestContext.

Instead, if we had:


  const net::URLRequestJobFactory* job_factory =
      loader->request()->context()->job_factory();

Then the job_factory() would be called on the same URLRequestContext as the originating URLRequest in the loader.

I can't test this out from home, but if anyone is still looking at this tonight, this might be a quick change to try out.

Comment 11 by dsb...@gmail.com, Sep 20 2012

applying this patch didn't fix - https://gist.github.com/3a07af22c41a7c4ae36c - same general errors

Comment 12 by tzik@chromium.org, Sep 20 2012

I wrote a test for it: http://codereview.chromium.org/10956009/
Looks like we're using the same job_factory as that of the main request context for Isolated apps?

In ProfileImplIOData::InitializeAppRequestContext():

  // Copy most state from the main context.
  AppRequestContext* context = new AppRequestContext(load_time_stats());
  context->CopyFrom(main_context);

This just copies the job_factory ptr to the context.
In InitializeURLRequestContext in storage_partition_impl_map.cc we return at line 150 before setting the *right* FileSystemProtocolHandler to the factory:

  if (job_factory->IsHandledProtocol(chrome::kBlobScheme))
    return;  // Already initialized this RequestContext.

Commenting out this line and forcing overwrite the protocol handler in url_request_job_factory_impl.cc makes it work.

I guess giving a new URLRequestJobFactory (with proper initialization) in InitializeAppRequestContext may fix the issue.



Thanks for the unittest tzik, and thanks for tracking down the error Kinuko!  That's a pretty tangled mess of initialization. :-/

I've created a patch to try and add a new URLRequestJobFactory for each isolated context.  Running through try bots now and will test with tzik's changes after I get into the office.

https://codereview.chromium.org/10969017

jhawkins: the patch is not tested, but if you want to pull it down locally to verify that it works, I give it 80% chance of success (assuming it compiles).


Comment 15 by dbeam@chromium.org, Sep 21 2012

The proposed patch seems to fix the bug AFAICT :)
Project Member

Comment 16 by bugdroid1@chromium.org, Sep 21 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=157900

------------------------------------------------------------------------
r157900 | ajwong@chromium.org | 2012-09-21T02:31:18.266077Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/net/chrome_url_request_context.h?r1=157900&r2=157899&pathrev=157900
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/profiles/profile_io_data.cc?r1=157900&r2=157899&pathrev=157900
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/profiles/profile_impl_io_data.cc?r1=157900&r2=157899&pathrev=157900
   A http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/api_test/xhr_persistent_fs/bg.js?r1=157900&r2=157899&pathrev=157900
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/profiles/profile_io_data.h?r1=157900&r2=157899&pathrev=157900
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/profiles/profile_impl_io_data.h?r1=157900&r2=157899&pathrev=157900
   A http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/api_test/xhr_persistent_fs/main.html?r1=157900&r2=157899&pathrev=157900
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/storage_partition_impl_map.cc?r1=157900&r2=157899&pathrev=157900
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/profiles/off_the_record_profile_io_data.cc?r1=157900&r2=157899&pathrev=157900
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_fileapi_apitest.cc?r1=157900&r2=157899&pathrev=157900
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/profiles/off_the_record_profile_io_data.h?r1=157900&r2=157899&pathrev=157900
   A http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/api_test/xhr_persistent_fs/manifest.json?r1=157900&r2=157899&pathrev=157900
   A http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/api_test/xhr_persistent_fs/main.js?r1=157900&r2=157899&pathrev=157900
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/net/chrome_url_request_context.cc?r1=157900&r2=157899&pathrev=157900
   A http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/api_test/xhr_persistent_fs?r1=157900&r2=157899&pathrev=157900

Create a new URLRequestJobFactory for isolated request contexts.

Also includes unittest from tzik@:
  http://codereview.chromium.org/10956009/

Originally the isolated URLRequestContexts used the same URLRequestJobFactory instance as the "default" request context.  This breaks filesystem: because the isolated context would incorrectly dispatch to FileSystemContext of the default URLRequestContext.

This CL makes it so the isolated contexts do not share the same URLRequestJobFactory.  There is now one URLRequestJobFactory per StoragePartition (the code equiv of one isolated context).  Note that each RequestContext and MediaRequestContext pair still share the same URLRequestJobFactory.  This is safe because they are in the isolation domain.

High level changes are:
- Each URLRequestJobFactory needs its own protocol_handler_interceptor
  which requires threading the parameter through all the
  URLRequestContext factory mess because this particular
  object must be created on the UI thread.

- GetIsolatedMediaRequestContext no longer looks up the
  app context out of the profile. Instead
  GetIsolatedMediaRequestContextGetter() does this. This
  makes it a little clearer that it is really a thin facade
  over the related isolated context.

- The common code for URLJobFactory creation is pulled
  up into SetUpJobFactoryDefaults out of both
  off_the_record and the normal profile_impl.  This will
  avoid future divergence of the setup.

- FtpProtocolHandler also moved into SetUpJobFactoryDefaults.
  Again, this is just to avoid future divergence.

- Lots of ownership passing moved to scoped_ptr<> to
  be more explicit.  No functionality change here, but lots
  of text churn.

TBR=finnur
BUG= 150861 

Review URL: https://codereview.chromium.org/10969017
------------------------------------------------------------------------
Labels: ReleaseBlock-Beta
@dbeam: Thanks for checking!

Adding the ReleaseBlock-Beta tag as it was left out before.

@jhawkins: if you can verify whether or not this unblocks you guys, that'd be great.

@rahulrc: Assuming we get a green there as well, we will need to get this merged in to m23 for the apps launch.  Can you help with coordinating that?

Comment 18 by dbeam@chromium.org, Sep 21 2012

#17 - yes, it unblocks our issues
Yep, fixed.  Thanks a lot!
Awesome.

Will look at merging tomorrow, probably in the afternoon (unless someone else decides to coordinate the merge before I get in).

Comment 21 by dbeam@chromium.org, Sep 21 2012

Labels: Merge-Requested
Was this issue resolved? I'm still receiving the same error as of 23.0.1271.1.

Comment 23 by ajwong@google.com, Sep 24 2012

23.0.1271.1 doesn't have the patch yet.  The next dev channel update should get it though.

Comment 24 by kareng@google.com, Sep 24 2012

Labels: -Merge-Requested Merge-Approved
r157900 broke filesystem: url access for component extensions (such as Chrome OS Files app).  Issue 151924  filed.
Project Member

Comment 26 by bugdroid1@chromium.org, Sep 24 2012

Labels: -Merge-Approved merge-merged-1271
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=158300

------------------------------------------------------------------------
r158300 | ajwong@chromium.org | 2012-09-24T17:21:33.293727Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/chrome/browser/profiles/profile_impl_io_data.h?r1=158300&r2=158299&pathrev=158300
   A http://src.chromium.org/viewvc/chrome/branches/1271/src/chrome/test/data/extensions/api_test/xhr_persistent_fs/main.html?r1=158300&r2=158299&pathrev=158300
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/content/browser/storage_partition_impl_map.cc?r1=158300&r2=158299&pathrev=158300
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/chrome/browser/profiles/off_the_record_profile_io_data.cc?r1=158300&r2=158299&pathrev=158300
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/chrome/browser/extensions/extension_fileapi_apitest.cc?r1=158300&r2=158299&pathrev=158300
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/chrome/browser/profiles/off_the_record_profile_io_data.h?r1=158300&r2=158299&pathrev=158300
   A http://src.chromium.org/viewvc/chrome/branches/1271/src/chrome/test/data/extensions/api_test/xhr_persistent_fs/manifest.json?r1=158300&r2=158299&pathrev=158300
   A http://src.chromium.org/viewvc/chrome/branches/1271/src/chrome/test/data/extensions/api_test/xhr_persistent_fs/main.js?r1=158300&r2=158299&pathrev=158300
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/chrome/browser/net/chrome_url_request_context.cc?r1=158300&r2=158299&pathrev=158300
   A http://src.chromium.org/viewvc/chrome/branches/1271/src/chrome/test/data/extensions/api_test/xhr_persistent_fs?r1=158300&r2=158299&pathrev=158300
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/chrome/browser/net/chrome_url_request_context.h?r1=158300&r2=158299&pathrev=158300
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/chrome/browser/profiles/profile_io_data.cc?r1=158300&r2=158299&pathrev=158300
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/chrome/browser/profiles/profile_impl_io_data.cc?r1=158300&r2=158299&pathrev=158300
   A http://src.chromium.org/viewvc/chrome/branches/1271/src/chrome/test/data/extensions/api_test/xhr_persistent_fs/bg.js?r1=158300&r2=158299&pathrev=158300
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/chrome/browser/profiles/profile_io_data.h?r1=158300&r2=158299&pathrev=158300

Merge 157900 - Create a new URLRequestJobFactory for isolated request contexts.

Also includes unittest from tzik@:
  http://codereview.chromium.org/10956009/

Originally the isolated URLRequestContexts used the same URLRequestJobFactory instance as the "default" request context.  This breaks filesystem: because the isolated context would incorrectly dispatch to FileSystemContext of the default URLRequestContext.

This CL makes it so the isolated contexts do not share the same URLRequestJobFactory.  There is now one URLRequestJobFactory per StoragePartition (the code equiv of one isolated context).  Note that each RequestContext and MediaRequestContext pair still share the same URLRequestJobFactory.  This is safe because they are in the isolation domain.

High level changes are:
- Each URLRequestJobFactory needs its own protocol_handler_interceptor
  which requires threading the parameter through all the
  URLRequestContext factory mess because this particular
  object must be created on the UI thread.

- GetIsolatedMediaRequestContext no longer looks up the
  app context out of the profile. Instead
  GetIsolatedMediaRequestContextGetter() does this. This
  makes it a little clearer that it is really a thin facade
  over the related isolated context.

- The common code for URLJobFactory creation is pulled
  up into SetUpJobFactoryDefaults out of both
  off_the_record and the normal profile_impl.  This will
  avoid future divergence of the setup.

- FtpProtocolHandler also moved into SetUpJobFactoryDefaults.
  Again, this is just to avoid future divergence.

- Lots of ownership passing moved to scoped_ptr<> to
  be more explicit.  No functionality change here, but lots
  of text churn.

TBR=finnur
BUG= 150861 

Review URL: https://codereview.chromium.org/10969017

TBR=ajwong@chromium.org
Review URL: https://codereview.chromium.org/10956076
------------------------------------------------------------------------
Cc: mihaip@chromium.org
@mihaip: This fixed the big breakage in filesystem:.  Adding you so you have full context on changes.

Comment 28 by kareng@google.com, Sep 24 2012

Labels: Merge-Merged
Status: Fixed
closing bug
Project Member

Comment 29 by bugdroid1@chromium.org, Mar 9 2013

Labels: -Type-Regression -Area-Internals -Feature-Apps -Mstone-23 Type-Bug-Regression Cr-Platform-Apps Cr-Internals M-23

Sign in to add a comment