Closed
Bug 889356
Opened 11 years ago
Closed 11 years ago
The redirects functionality appears to fail via pushing the app to the device or via the simulator
Categories
(Firefox OS Graveyard :: General, defect)
Firefox OS Graveyard
General
Tracking
(blocking-b2g:leo+, firefox23 wontfix, firefox24 wontfix, firefox25 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)
People
(Reporter: mbtyke, Assigned: ochameau)
References
Details
(Whiteboard: QARegressExclude)
Attachments
(4 files, 1 obsolete file)
(deleted),
patch
|
fabrice
:
review+
|
Details | Diff | Splinter Review |
(deleted),
application/zip
|
Details | |
(deleted),
patch
|
ochameau
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ochameau
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_4) AppleWebKit/536.30.1 (KHTML, like Gecko) Version/6.0.5 Safari/536.30.1
Steps to reproduce:
I have a sample app at
https://github.com/michaeljbishop/b2g-sample-apps/tree/master/redirects
which does a simple redirects test. The webapp shows two buttons, both should navigate to the same page, but in a different way.
The first button opens up the target page via redirection, the second button opens it directly.
Actual results:
The redirection button shows an error in the simulator which says "about:// is not loading properly"
Expected results:
I would have expected to see the target page (whose content simply shows "SUCESS!!"
Comment 1•11 years ago
|
||
I can reproduce on the simulator, but I can't reproduce this behavior on device. On device, nothing happens when clicking both links.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 2•11 years ago
|
||
Ah, I know why this is happening. The simulator isn't ensuring that the CSP policy is enforced. The app provided here isn't a valid app - you are not allowed to have inline JS in privileged apps.
Comment 3•11 years ago
|
||
Migrating this over to https://github.com/mozilla/r2d2b2g/issues/685. I think this is a simulator bug you are seeing.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → INVALID
Well, I changed the source code (same link) so that the javascript was no longer inline.
The behavior changes, but it's still shows a similar problem.
I've added more to the discussion here...
https://groups.google.com/d/msg/mozilla.dev.b2g/lgZ4Ivjl6Yk/em_yf8-5h0AJ
Essentially, I removed the inline javascript and set up a local web-server that should send a HTTP 302 to the app running in the simulator. The app should see that and re-redirect to the local file. At least, that's what I've been led to believe :)
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Comment 5•11 years ago
|
||
Not based on what I'm looking at.
<input type="button" value="Open via redirection" onclick="testRedirectSource()"/>
<input type="button" value="Open directly" onclick="testRedirectTarget()"/>
That's inline JS right there. You can't use the onXXX handlers directly in tags.
Comment 6•11 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #5)
> Not based on what I'm looking at.
>
> <input type="button" value="Open via redirection"
> onclick="testRedirectSource()"/>
> <input type="button" value="Open directly" onclick="testRedirectTarget()"/>
>
> That's inline JS right there. You can't use the onXXX handlers directly in
> tags.
Should say - you can't use direct JS or onXXX handlers in the HTML files.
Thank you for the clarification. I didn't know that.
I've modified the source again to be what I believe is totally clear of inline Javascript code.
Unfortunately, I'm receiving the same error as before.
Here is the removed Javascript.
https://github.com/michaeljbishop/b2g-sample-apps/commit/f982d1eb4dff0713bef3a9fb83cdd252c81a879b
Comment 8•11 years ago
|
||
Manifest "redirects" depend on HTTP redirects and are ignored otherwise. I added a Note to the docs @ https://developer.mozilla.org/en-US/docs/Web/Apps/Manifest#redirects
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → INVALID
It pains me to reopen this but, I don't think we can consider this bug closed. My example does indeed use a redirect. The flow is like this:
1. App calls window.open with http://michaelpro.local/~mbishop/index.html
2. michaelpro.local responds with
HTTP/1.1 302 Found
Date: Tue, 02 Jul 2013 21:16:37 GMT
Server: Apache/2.2.22 (Unix) DAV/2 mod_ssl/2.2.22 OpenSSL/0.9.8x
Location: app://firefoxos.non-existent-domain-asdfg.com/authenticated.html
Content-Type: text/html; charset=iso-8859-1
3. The app has the manifest entry:
"origin": "app://firefoxos.non-existent-domain-asdfg.com",
"name": "Test Redirection",
"version": "0.0.1",
"description": "Test Redirection inside Firefox OS",
"locales": {
"en_US": {
"name": "Test Redirection"
}
},
"type": "privileged",
"permissions": {
"systemXHR": {}
},
"redirects": [
{"from": "app://firefoxos.non-existent-domain-asdfg.com/authenticated.html",
"to": "/redirects/authenticated.html"}
]
4. At this point, I'd expect app:///redirects/authenticated.html to be loaded.
I've received feedback that I should change the "redirects" entry to use 'http' and change the michaelpro.local webserver to correspondingly redirect to an 'http" entry as well. I've tried that and I receive the same behavior.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Harald, others -- if Michael is doing something wrong, can you take his example and modify it so that it works according to what's implemented currently? It seems like there's been a lot of things suggested, which leads me to believe this is a complicated area. We should instead present a working example instead of suggesting things that end up not working out :)
Comment 11•11 years ago
|
||
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #10)
> Harald, others -- if Michael is doing something wrong, can you take his
> example and modify it so that it works according to what's implemented
> currently? It seems like there's been a lot of things suggested, which
> leads me to believe this is a complicated area. We should instead present a
> working example instead of suggesting things that end up not working out :)
I'll work on writing up a working example.
Reporter | ||
Comment 12•11 years ago
|
||
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #10)
> Harald, others -- if Michael is doing something wrong, can you take his
> example and modify it so that it works according to what's implemented
> currently? It seems like there's been a lot of things suggested, which
> leads me to believe this is a complicated area. We should instead present a
> working example instead of suggesting things that end up not working out :)
This is a great idea.
Comment 13•11 years ago
|
||
I reduced the test case and added a remote node.js server that just takes a single query argument and redirects to that URL.
https://github.com/digitarald/b2g-sample-apps
It doesn't work in Simulator or pushed to a 1.0.1 Unagi via Simulator. My only idea is that the Simulator push doesn't register the `redirects`, but that's a wild guess.
Have you tried installing it directly on the Unagi via an install page?
Comment 15•11 years ago
|
||
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #14)
> Have you tried installing it directly on the Unagi via an install page?
Doesn't work with privileged apps. I'll try tomorrow to push it to the phone using it as preload in a gaia build.
Reporter | ||
Comment 16•11 years ago
|
||
This is blocking partner contract work for Firefox OS.
Severity: normal → major
Comment 18•11 years ago
|
||
(In reply to Harald Kirschner :digitarald from comment #17)
> Any updates from your testing?
Not yet. I'll try to get to this today.
Flags: needinfo?(jsmith)
Assignee | ||
Comment 19•11 years ago
|
||
I looked at the code, and it seems that we just omit redirects when we install an app through the devtool's actor. (This feature has been added late and we forgot about backporting it to the actor)
So it is most likely going to be broken with the simulator and any tool based on the devtool push-to-device feature.
We can fix that pretty shortly on the simulator with weekly preview build,
but it will stay broken on 1.0.1 devices and will require requesting an uplift if we want to see that fixed for 1.1 devices.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → poirot.alex
Comment 20•11 years ago
|
||
I'll look into checking the non-dev tool workflow.
This is blocking partner development work on 1.1 however (as this does not work with our dev tools), so noming for leo.
blocking-b2g: --- → leo?
Updated•11 years ago
|
Summary: The "redirects" functionality appears to fail → The redirects functionality appears to fail via pushing the app to the device or via the simulator
Comment 21•11 years ago
|
||
(In reply to Alexandre Poirot (:ochameau) from comment #19)
> I looked at the code, and it seems that we just omit redirects when we
> install an app through the devtool's actor. (This feature has been added
> late and we forgot about backporting it to the actor)
> So it is most likely going to be broken with the simulator and any tool
> based on the devtool push-to-device feature.
> We can fix that pretty shortly on the simulator with weekly preview build,
> but it will stay broken on 1.0.1 devices and will require requesting an
> uplift if we want to see that fixed for 1.1 devices.
I'd vote for a 1.1 uplift, as it affects partners and an unreported number of developers who might have tried and failed adding FB, Twitter, etc authentication. As it is very unexpected and leads into trying a lot of workaround, it can cause a great deal of frustration.
Assignee | ||
Comment 22•11 years ago
|
||
Jason, is there any bug being opened specific to the CSP issue?
We are having similar issue for CSP, when we push a privileged app, the CSP are not correctly set. As for this bug, I'll fix that in simulator shortly, but we should also fix that on devices.
Comment 23•11 years ago
|
||
(In reply to Alexandre Poirot (:ochameau) from comment #22)
> Jason, is there any bug being opened specific to the CSP issue?
> We are having similar issue for CSP, when we push a privileged app, the CSP
> are not correctly set. As for this bug, I'll fix that in simulator shortly,
> but we should also fix that on devices.
Nope, I don't think there's a bug filed for that. Feel free to open a bug on that.
Assignee | ||
Comment 24•11 years ago
|
||
Attachment #772753 -
Flags: review?(fabrice)
Comment 25•11 years ago
|
||
Given where this code lies, there's no risk to a user use case and this semi-blocks partner development, so we'll leo+ (and mistakenly mark as NPOTB)
blocking-b2g: leo? → leo+
Whiteboard: [NPOTB]
Assignee | ||
Comment 26•11 years ago
|
||
You can test that patch with this packaged app.
That tries to load `http://test.techno-barje.fr/manifests/redirect/index.html`
Where I have the following htaccess being registered:
```
Redirect permanent /manifests/redirect/index.html http://test.techno-barje.fr/manifests/redirect/redirected.html
```
Updated•11 years ago
|
Attachment #772753 -
Flags: review?(fabrice) → review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 27•11 years ago
|
||
Assignee | ||
Comment 28•11 years ago
|
||
Comment on attachment 773679 [details] [diff] [review]
Add test for app `redirects` manifest feature.
It looks like we can actually test that complex feature with a good old xpcshell test. I wish we could have mochitest-chrome or something similar for b2g,
that would ease testing apps in real environment... Having createWindowlessBrowser() or any way to get a working window via xpcshell would also open ways for better tests.
This patch add a new test: /toolkit/devtools/apps/tests/unit/test_appInstall.js
while factorizing some code with the existing test_webappsActor.js
Attachment #773679 -
Flags: review?(fabrice)
Comment 29•11 years ago
|
||
Gonna hold off on landing this until the test has r+. Please put checkin-needed back on when ready.
Keywords: checkin-needed
Comment 30•11 years ago
|
||
Comment on attachment 773679 [details] [diff] [review]
Add test for app `redirects` manifest feature.
Review of attachment 773679 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/devtools/apps/tests/unit/head_apps.js
@@ +37,5 @@
> + if (!gActor) {
> + connect(webappActorRequest.bind(null, request, onResponse));
> + return;
> + }
> +
nit: trailing white space
::: toolkit/devtools/apps/tests/unit/test_appInstall.js
@@ +54,5 @@
> + let d = Cc["@mozilla.org/docshell;1"].createInstance(Ci.nsIDocShell);
> + d.QueryInterface(Ci.nsIWebNavigation);
> + d.QueryInterface(Ci.nsIWebProgress);
> +
> + let localAppId = appsService.getAppLocalIdByManifestURL(APP_MANIFEST);
nit: two spaces after '='
Attachment #773679 -
Flags: review?(fabrice) → review+
Assignee | ||
Comment 31•11 years ago
|
||
Attachment #773679 -
Attachment is obsolete: true
Attachment #774100 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 32•11 years ago
|
||
https://hg.mozilla.org/projects/birch/rev/a28690ed91ef
https://hg.mozilla.org/projects/birch/rev/b22b41460ba3
Flags: in-testsuite+
Keywords: checkin-needed
Comment 33•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a28690ed91ef
https://hg.mozilla.org/mozilla-central/rev/b22b41460ba3
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Comment 34•11 years ago
|
||
Can this be uplifted to 1.1? It's leo+ and the patch is r+
Comment 35•11 years ago
|
||
Removing NPOTB so that this gets uplifted. comment 25 was to keep it out of our queries.
status-b2g18:
--- → affected
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-b2g-v1.1hd:
--- → affected
Whiteboard: [NPOTB]
Comment 36•11 years ago
|
||
Needs a branch-specific patch for uplift.
status-firefox23:
--- → wontfix
status-firefox24:
--- → wontfix
status-firefox25:
--- → fixed
Flags: needinfo?(poirot.alex)
Keywords: branch-patch-needed
Target Milestone: --- → 1.1 QE4 (15jul)
Assignee | ||
Comment 37•11 years ago
|
||
Here is the patch for b2g18. Unfortunately, tests depends on another bug that hasn't been uplifted (bug 844227).
Attachment #778998 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(poirot.alex)
Keywords: branch-patch-needed → checkin-needed
Updated•11 years ago
|
Comment 38•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/68fb0a2e0114
Feel free to put a checkin-needed on this for the tests if/when bug 844227 gets approval for uplift.
Keywords: checkin-needed
Comment 39•11 years ago
|
||
Updated•11 years ago
|
Whiteboard: QARegressExclude
You need to log in
before you can comment on or make changes to this bug.
Description
•