Open Bug 827243 Opened 12 years ago Updated 2 years ago

async XMLHttpRequest send() throws NS_ERROR_FILE_NOT_FOUND on AsyncOpen for app:// protocol instead of generating a 404

Categories

(Core :: Networking: JAR, defect, P5)

Product:
Core
Component:
Networking: JAR
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Project Flags:
blocking-basecamp -
Tracking Flags:
Tracking Status
b2g18 + ---

People

(Reporter: asuth, Unassigned)

References

Details

(Whiteboard: [necko-would-take]) 

Potential regression from bug 815523 that affected the Gaia e-mail app, now worked around on bug 826468.  The ::Send method's synchronous invocation of AsyncOpen returns NS_ERROR_FILE_NOT_FOUND after consulting the jar cache.

What is currently happening is that xhr.send() is throwing an exception:
[Exception... "File error: Not found"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: app://email.gaiamobile.org/js/ext/gaia-email-opt.js :: getXmlConfig :: line 35948"  data: no]

Discussion on dev-b2g:
https://groups.google.com/d/msg/mozilla.dev.b2g/LZFt1_Tf9u8/WbqXocmmiQgJ

Requesting tracking for b2g since this may affect other apps.
Blocks: 815523
No longer depends on: 815523
Probably not blocking unless this is a regression.
Assignee: nobody → jduell.mcbugs
blocking-basecamp: --- → ?
It's definitely a regression because send() never used to throw in the exact same usage pattern with the app:// protocol, but now it does.  But the e-mail use-case is likely to be rare; most of the time you won't use an XHR to see if something is there in a packaged app.
Is the protocol handler just throwing from asyncOpen?  Is the bug about fixing the protocol handler or to work around it in XHR?
A testcase would save me some time here.

Bug 815523 didn't change how Jar cache is searched, so I'm surprised by this.  But of course it's possible that it broke things.

I'll look into this more once I've got actual blockers done.
Let's mark this as a nice-to-have soft blocker.
blocking-basecamp: ? → -
tracking-b2g18: ? → +
See Also: → 857393
Marcos: Can you make sure that the app:// spec specifies that using XHR to load from a non-existent app:// URL should yield a 404 result. And that loading using an <iframe> should result in the "load" even firing successfully.

I.e. we should always behave like a 404, and not like an invalid URL or similar.
(In reply to Jonas Sicking (:sicking) from comment #6)
> Marcos: Can you make sure that the app:// spec specifies that using XHR to
> load from a non-existent app:// URL should yield a 404 result. And that
> loading using an <iframe> should result in the "load" even firing
> successfully.
> 
> I.e. we should always behave like a 404, and not like an invalid URL or
> similar.

In section "6.4 Dereferencing and retrieval of files from a container" [1], step 9, it states that:

"If potential-file is not found at the given path inside the container, return a [HTTP] 404 Not Found response. "

Happy to clarify the algorithm further if that is insufficient (e.g., by adding a non-normative note about how this behavior would map to HTML).   

[1] http://app-uri.sysapps.org/#dereferencing-and-retrieval-of-files-from-a-container
You probably also need to define that the request returns an empty response body in addition to the 404 status.
(In reply to Jonas Sicking (:sicking) from comment #8)
> You probably also need to define that the request returns an empty response
> body in addition to the 404 status.

I've updated the spec to say: 

A response means a HTTP response and can include any HTTP response fields (e.g., the name of the user agent as the Server:) and any entity header fields the user agent deems will be helpful to a developer (e.g., Content-Length, Last-Modified, Date, Content-Encoding, Content-MD5). In the case of an error in the request (i.e., status codes 500, 501, 400, 404), the user agent MUST return an empty response body. Optionally, in case of an error in the request, the user agent SHOULD provide the developer with feedback indicating what error occurred (e.g., in an error console or where network request information is available via the user agent's developer tools).
Is there any update with this bug? In FTU App for example we are retrieving this error when a customization configuration file is not available and it's requested via XHR, and we would like to control this through the regular 404 error.
Some answers to the questions from comment 3 and comment 4 would help here.
Anyone have a simple test case that shows this?
Quite easy to check:

var xhr = new XMLHttpRequest();
xhr.open('GET', 'wrong_file_path', true); 
xhr.responseType = 'json';
xhr.onload = function() {
  // Any code here...
};
    
xhr.send();

If we are pointing to a resource which is not available, you will see a log as:
E/GeckoConsole(  626): [JavaScript Error: "NS_ERROR_FILE_NOT_FOUND: " {file: "wrong_file_path" line: ...}]
(In reply to Boris Zbarsky [:bz] from comment #3)
> Is the protocol handler just throwing from asyncOpen?  Is the bug about
> fixing the protocol handler or to work around it in XHR?

I don't think we should work around this in the xhr code. It'd likely come back and bite us for things like <IMG> or <iframe> pool pointing at a non-existing app: resource.
OK, so looking at the impl in netwerk/protocol/app/AppProtocolHandler.js (which I assume is the relevant bit), it looks like it's just returning a jar: channel, right?  In particular, a jar:file:// channel.

Then we land in nsJARChannel::AsyncOpen.  LookupFile presumably succeeds, and then we do nsJARChannel::OpenLocalFile.  This calls nsJARChannel::CreateJarInput which lands us in the jar cache which can in fact return NS_ERROR_FILE_NOT_FOUND.

The jar channel then propagates this out, instead of canceling self with the error code, which is what it should do.
Component: DOM → Networking: JAR
We have code in nsXMLHttpRequest that returns 404 when we hit NS_ERROR_FILE_NOT_FOUND:
https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#1097
That doesn't matter.  This bug is about send() throwing, not what .status returns after that.
Jason, any plans wrt this? We have to trick it around in L10n lib for FxOS
Flags: needinfo?(jduell.mcbugs)
bz: re: comment 15, do you think we should convert just NS_ERROR_FILE_NOT_FOUND to a Cancel (instead of a synchronous failure), or would we want to do that for pretty much any error we get from OpenLocalFile?
Flags: needinfo?(bzbarsky)
Daniel,

You're working on some app:// protocol related stuff, so I'm going to give this to you.

I suspect bz is right in comment 15.  (Note that this means that the XHR channel is in the parent process, not on a child, so we're not using remoteopenfile).  IIUC what we'd need to do here is to change the code in nsJARChannel::ContinueAsyncOpen() so that right after it calls OpenLocalFile, if rv==NS_ERROR_FILE_NOT_FOUND (or perhaps for any NS_FAILED(rv)--lets see what bz says about that) into the equivalent of a Cancel calls (i.e instead of having AsyncOpen return with an errror, we return NS_OK but then call OnStart/OnStop with the error status set to FILE_NOT_FOUND (which comment 16 indicates is then handled by XHR).

Since we don't have any kind of inputstreampump to just cancel() (we couldn't open the file), we'll need something like the AsyncAborter logic in HttpBaseChannel (except I doubt it needs to be a template: we just need to create an object that will call OnStart/Stop with the correct error code, and dispatch it to the main thread so it runs after AsyncOpen returns).

Ping me if that's unclear.  This isn't an urgent issue but we should fix.
Assignee: jduell.mcbugs → daniel
Flags: needinfo?(jduell.mcbugs) → needinfo?(daniel)
Oh, and FWIW, I can't figure out how this is a regression.  I looked over bug 815523 and AFAICT that bug only changed logic for the remote file case, not the local file case.  But maybe I missed something, and it doesn't really matter.
I generally think that we should never throw from AsyncOpen, ever, and should instead just report all errors async.  That simplifies behavior greatly for consumers and gets rid of these perennial problems of non-spec-defined exceptions leaking out onto the web in various ways...
Flags: needinfo?(bzbarsky)
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #21)
> Oh, and FWIW, I can't figure out how this is a regression.  I looked over
> bug 815523 and AFAICT that bug only changed logic for the remote file case,
> not the local file case.  But maybe I missed something, and it doesn't
> really matter.

Note that I was speculatively pointing fingers at that bug based on a cursory investigation, but I think it was clear it was a regression because the email use-case had always been to do a local XHR to look for an autoconfig file in our local app zip before performing a network ISPDB lookup.  This worked when originally landed and tested, and then it broke at some point, so something absolutely changed somewhere along the code path that services XHR requests over the app protocol.
Flags: needinfo?(daniel)
Whiteboard: [necko-would-take]
Assignee: daniel → nobody
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P5
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.