Last Comment Bug 804395 - app:// protocol and XHR
: app:// protocol and XHR
Status: RESOLVED FIXED
: addon-compat
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla19
Assigned To: Andrea Marchesini [:baku]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on: 807508
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-22 16:23 PDT by David Flanagan [:djf]
Modified: 2013-01-03 14:54 PST (History)
12 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
fixed
fixed


Attachments
patch 1 (5.12 KB, patch)
2012-10-23 04:29 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 2 (6.09 KB, patch)
2012-10-23 18:27 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 3 (6.51 KB, patch)
2012-10-24 01:25 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 4 (6.92 KB, patch)
2012-10-24 07:01 PDT, Andrea Marchesini [:baku]
jonas: review+
Details | Diff | Splinter Review
patch 5 (6.92 KB, patch)
2012-10-24 11:34 PDT, Andrea Marchesini [:baku]
amarchesini: review+
Details | Diff | Splinter Review
patch 5b (6.95 KB, patch)
2012-10-24 11:36 PDT, Andrea Marchesini [:baku]
amarchesini: review+
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description David Flanagan [:djf] 2012-10-22 16:23:42 PDT
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] 2012-10-22 16:25:15 PDT
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] 2012-10-23 04:29:30 PDT
Created attachment 674197 [details] [diff] [review]
patch 1

This is just a proposal.
Comment 3 Andrea Marchesini [:baku] 2012-10-23 05:57:19 PDT
https://tbpl.mozilla.org/?tree=Try&rev=0940fbbddcc3 green on try
Comment 4 David Flanagan [:djf] 2012-10-23 09:37:24 PDT
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 2012-10-23 10:22:41 PDT
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] 2012-10-23 10:27:13 PDT
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] 2012-10-23 18:27:15 PDT
Created attachment 674479 [details] [diff] [review]
patch 2

Jonas, feedback about this?
Comment 8 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-23 19:53:36 PDT
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] 2012-10-24 01:25:47 PDT
Created attachment 674587 [details] [diff] [review]
patch 3
Comment 10 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-24 06:40:26 PDT
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] 2012-10-24 07:01:27 PDT
Created attachment 674648 [details] [diff] [review]
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] 2012-10-24 10:46:34 PDT
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 2012-10-24 11:14:44 PDT
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] 2012-10-24 11:34:26 PDT
Created attachment 674760 [details] [diff] [review]
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] 2012-10-24 11:36:17 PDT
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:
Comment 16 bhavana bajaj [:bajaj] 2012-10-24 16:28:44 PDT
(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] 2012-10-24 17:01:26 PDT
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] 2012-10-24 17:13:03 PDT
(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] 2012-10-24 17:40:22 PDT
> 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] 2012-10-25 18:58:41 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/02647d980da2
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-10-26 04:34:50 PDT
https://hg.mozilla.org/mozilla-central/rev/02647d980da2
Comment 22 Ryan VanderMeulen [:RyanVM] 2012-10-27 17:05:39 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/629284f58552
Comment 23 Dave Townsend [:mossop] 2012-10-31 16:23:49 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.