The default bug view has changed. See this FAQ.

app:// protocol and XHR

RESOLVED FIXED in Firefox 18

Status

()

Core
Networking
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: djf, Assigned: baku)

Tracking

({addon-compat})

Trunk
mozilla19
x86
Mac OS X
addon-compat
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking-basecamp:-, firefox18 fixed, firefox19 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
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.
blocking-basecamp: --- → ?
(Assignee)

Comment 2

5 years ago
Created attachment 674197 [details] [diff] [review]
patch 1

This is just a proposal.
Attachment #674197 - Flags: review?(dflanagan)
(Assignee)

Comment 3

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=0940fbbddcc3 green on try
(Reporter)

Comment 4

5 years ago
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?
(Assignee)

Updated

5 years ago
Attachment #674197 - Flags: review?(dflanagan) → review?(jonas)
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.
(Assignee)

Comment 6

5 years ago
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 :)
(Assignee)

Comment 7

5 years ago
Created attachment 674479 [details] [diff] [review]
patch 2

Jonas, feedback about this?
Attachment #674197 - Attachment is obsolete: true
Attachment #674197 - Flags: review?(jonas)
Attachment #674479 - Flags: review?(jonas)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
Created attachment 674587 [details] [diff] [review]
patch 3
Attachment #674479 - Attachment is obsolete: true
Attachment #674479 - Flags: review?(jonas)
Attachment #674587 - Flags: review?(josh)
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?
Attachment #674587 - Flags: review?(josh) → review?(jonas)
(Assignee)

Comment 11

5 years ago
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.
Attachment #674587 - Attachment is obsolete: true
Attachment #674587 - Flags: review?(jonas)
Attachment #674648 - Flags: review?(jonas)
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.
blocking-basecamp: ? → -
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.
Attachment #674648 - Flags: review?(jonas) → review+
(Assignee)

Comment 14

5 years ago
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:
Attachment #674648 - Attachment is obsolete: true
Attachment #674760 - Flags: review+
Attachment #674760 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 15

5 years ago
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:
Attachment #674760 - Attachment is obsolete: true
Attachment #674760 - Flags: approval-mozilla-aurora?
Attachment #674763 - Flags: review+
Attachment #674763 - Flags: approval-mozilla-aurora?
(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 !
(Assignee)

Comment 17

5 years ago
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.
(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 ?
(Assignee)

Comment 19

5 years ago
> 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
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/02647d980da2
Assignee: nobody → amarchesini
Flags: in-testsuite-
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/02647d980da2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Updated

5 years ago
Attachment #674763 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/629284f58552
status-firefox18: --- → fixed
status-firefox19: --- → fixed
Depends on: 807508
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.
Keywords: addon-compat
You need to log in before you can comment on or make changes to this bug.