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)

defect
Not set
major

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)

RESOLVED FIXED
1.1 QE4 (15jul)
blocking-b2g leo+
Tracking Status
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)

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!!"
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
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.
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 → ---
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.
(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
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 ago11 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 :)
(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.
(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.
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?
(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.
This is blocking partner contract work for Firefox OS.
Severity: normal → major
Any updates from your testing?
Flags: needinfo?(jsmith)
(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)
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: nobody → poirot.alex
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?
Summary: The "redirects" functionality appears to fail → The redirects functionality appears to fail via pushing the app to the device or via the simulator
(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.
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.
(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.
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]
Attached file Test app (deleted) —
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 ```
Attachment #772753 - Flags: review?(fabrice) → review+
Keywords: checkin-needed
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)
Gonna hold off on landing this until the test has r+. Please put checkin-needed back on when ready.
Keywords: checkin-needed
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+
Attachment #773679 - Attachment is obsolete: true
Attachment #774100 - Flags: review+
Keywords: checkin-needed
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Can this be uplifted to 1.1? It's leo+ and the patch is r+
Removing NPOTB so that this gets uplifted. comment 25 was to keep it out of our queries.
Needs a branch-specific patch for uplift.
Flags: needinfo?(poirot.alex)
Target Milestone: --- → 1.1 QE4 (15jul)
Here is the patch for b2g18. Unfortunately, tests depends on another bug that hasn't been uplifted (bug 844227).
Attachment #778998 - Flags: review+
Flags: needinfo?(poirot.alex)
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.
Whiteboard: QARegressExclude
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: