804395 - app:// protocol and XHR

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[David Flanagan [:djf]]

Third-party app developers for FirefoxOS will expect XHR on application resources using the app:// protocol to work like it does for http:// URLs.

Currently, however, it does not seem to work that way.

My test environment was using synchronous requests in a worker, with responseType set to "arraybuffer" so these bugs may not be general.  What I observed, however:

1) successful requests had a status field of 0.  Lots of developers are trained to check for a status of 200.  Blob urls emulate http status codes, and we need to make it easy for developers to write code that works unpackaged with http urls and packaged with app urls.  This piece of the bug seems like a specification and usability issue.

2) when I requested a file that did not exist, the request was successful. xhr.response was a valid array buffer, and my only indication that something was wrong was that the array buffer had a 0 length. This part of the bug seems like an implementation issue.

Sorry I don't have a reduced test case to attach right now.

Comment 1

[David Flanagan [:djf]]

I'm nominating this as blocking because it would be pretty bad for 3rd party developers if XHR requests that work with http was to break when packaged up as an app.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: patch 1

This is just a proposal.

Comment 3

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=0940fbbddcc3 green on try

Comment 4

[David Flanagan [:djf]]

Andrea,

That was fast work!  Unfortunately, I'm not qualified to review it. I don't know anything about the codebase, and my C++ is weak.

Maybe Fabrice would review the patch?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think we need something slightly more complicated here. We should only return 200 if the load was successful and there was actually a file to load.

If you load something from app:// and there is no file with that URL we should return 404.

If you load something from app:// and we had an error somewhere, other than the file-not-found problem, we should return 500.

Comment 6

[Andrea Marchesini [:baku]]

Makes sense. Right now just the first point is done:

> I think we need something slightly more complicated here. We should only
> return 200 if the load was successful and there was actually a file to load.

These 2 are missing:

> If you load something from app:// and there is no file with that URL we
> should return 404.
> If you load something from app:// and we had an error somewhere, other than
> the file-not-found problem, we should return 500.

Soon a new patch :)

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch 2

Jonas, feedback about this?

Comment 8

[Josh Matthews [:jdm]]

Comment on attachment 674479 [details] [diff] [review]
patch 2

>+already_AddRefed<nsIJARChannel>
>+nsXMLHttpRequest::GetCurrentJARChannel()
>+{
>+  nsIJARChannel *appChannel = nullptr;
>+
>+  if (mReadRequest) {
>+    CallQueryInterface(mReadRequest, &appChannel);
>+  }
>+
>+  if (!appChannel && mChannel) {
>+    CallQueryInterface(mChannel, &appChannel);
>+  }
>+
>+  return appChannel;
>+}

Please use

>nsCOMPtr<nsIJarChannel> appChannel = do_QueryInterface(mReadRequest);
>if (!appChannel) {
>  appChannel = do_QueryInterface(mChannel);
>}
>return appChannel.forget();

instead.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: patch 3

Comment 10

[Josh Matthews [:jdm]]

Comment on attachment 674587 [details] [diff] [review]
patch 3

Review of attachment 674587 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsXMLHttpRequest.cpp
@@ +1240,5 @@
>    }
>  
>    uint16_t readyState;
>    GetReadyState(&readyState);
>    if (readyState == UNSENT || readyState == OPENED || mErrorLoad) {

Won't this mean that doing something like

>var xhr = SpecialPowers.createSystemXHR();
>xhr.open('GET', 'jar:http://example.org/tests/content/base/test/file_bug804395.jar!/foo.bar', true);
>console.log(xhr.status)

will display 500?

Comment 11

[Andrea Marchesini [:baku]]

Attachment: patch 4

Good point, Josh.
I added a new test case and now it returns 0 if the request has not been sent.

Comment 12

[Andrew Overholt [:overholt]]

This was discussed during triage and while we'd like to take the patch (and will aurora+), we wouldn't hold the release for it.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 674648 [details] [diff] [review]
patch 4

Review of attachment 674648 [details] [diff] [review]:
-----------------------------------------------------------------

Please nominate this for aurora too. Since this affects all jar channels it has a non-zero but very low risk for non-B2G builds I don't want to plus it myself. Definitely worth taking in my opinion since the risk is so low and it'll be a nice fix for B2G.

::: content/base/src/nsXMLHttpRequest.cpp
@@ +1261,5 @@
> +
> +    return 0;
> +  }
> +
> +  // Let's simulate the http protocol for jar/app requests:

Move this into the if(!httpChannel) branch.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: patch 5

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 15

[Andrea Marchesini [:baku]]

Attachment: patch 5b

forgot to do hg qref. Sorry.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 16

[bhavana bajaj [:bajaj]]

(In reply to Andrea Marchesini (:baku) from comment #15)
> Created attachment 674763 [details] [diff] [review]
> patch 5b
> 
> forgot to do hg qref. Sorry.
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
> User impact if declined: 
> Testing completed (on m-c, etc.): 
> Risk to taking this patch (and alternatives if risky): 
> String or UUID changes made by this patch:

Can you please help us out by filling the above information before we can approve the patch ? Thanks !

Comment 17

[Andrea Marchesini [:baku]]

Right. I have answers for these 2 points:

[Approval Request Comment]
User impact if declined: Yes, Third-party app developers for FirefoxOS will expect XHR on application resources using the app:// protocol to work like it does for http:// URLs.

Testing completed (on m-c, etc.): m-c mochitest is part of this patch.

Comment 18

[bhavana bajaj [:bajaj]]

(In reply to Andrea Marchesini (:baku) from comment #17)
> Right. I have answers for these 2 points:
> 
> [Approval Request Comment]
> User impact if declined: Yes, Third-party app developers for FirefoxOS will
> expect XHR on application resources using the app:// protocol to work like
> it does for http:// URLs.
> 
> Testing completed (on m-c, etc.): m-c mochitest is part of this patch.

Thanks ! One last question, comment 13 suggest it has a " very low risk for non-B2G builds " . I was more worried about the regressions it might cause on the Desktop/mobile builds . What is the worst that could happen if this patch was approved on that front ?

Comment 19

[Andrea Marchesini [:baku]]

> Thanks ! One last question, comment 13 suggest it has a " very low risk for
> non-B2G builds " . I was more worried about the regressions it might cause
> on the Desktop/mobile builds . What is the worst that could happen if this
> patch was approved on that front ?

If we have some XHR working with jar: or app: URIs, now xhr.status has http values (200, 404, 500). Previously it was always 0. If some code was checking the value of xhr.status, that code will break... but it was 0 before, so, probably nobody is relying on it

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02647d980da2

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/02647d980da2

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/629284f58552

Comment 23

[Dave Townsend [:mossop]]

I'm concerned about potential add-on bustage from this. It broke the Jetpack automated tests and though looks like just a test problem (i.e. we aren't in a place where all SDK based add-ons will necessarily break) I wonder if other add-on developers are relying on the status being 0 for local-like resources. I think I've seen example code test for that.

A brief search on AMO is suggesting that many cases are testing for both 200 and 0, so that's good, but that wasn't an exhaustive search.