889356 - The redirects functionality appears to fail via pushing the app to the device or via the simulator

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[mbtyke]

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

[Jason Smith [:jsmith]]

I can reproduce on the simulator, but I can't reproduce this behavior on device. On device, nothing happens when clicking both links.

Comment 2

[Jason Smith [:jsmith]]

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

[Jason Smith [:jsmith]]

Migrating this over to https://github.com/mozilla/r2d2b2g/issues/685. I think this is a simulator bug you are seeing.

Comment 4

[mbtyke]

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 :)

Comment 5

[Jason Smith [:jsmith]]

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

[Jason Smith [:jsmith]]

(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.

Comment 7

[mbtyke]

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

[:Harald Kirschner :digitarald]

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

Comment 9

[mbtyke]

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.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

[Jason Smith [:jsmith]]

(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.

Comment 12

[mbtyke]

(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

[:Harald Kirschner :digitarald]

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.

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Have you tried installing it directly on the Unagi via an install page?

Comment 15

[:Harald Kirschner :digitarald]

(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.

Comment 16

[mbtyke]

This is blocking partner contract work for Firefox OS.

Comment 17

[:Harald Kirschner :digitarald]

Any updates from your testing?

Comment 18

[Jason Smith [: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.

Comment 19

[Alexandre Poirot [:ochameau]]

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.

Comment 20

[Jason Smith [:jsmith]]

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.

Comment 21

[:Harald Kirschner :digitarald]

(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.

Comment 22

[Alexandre Poirot [:ochameau]]

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

[Jason Smith [:jsmith]]

(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.

Comment 24

[Alexandre Poirot [:ochameau]]

Attachment: Ensure setting redirects property on app object in the actor

Comment 25

[Alex Keybl [:akeybl]]

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)

Comment 26

[Alexandre Poirot [:ochameau]]

Attachment: Test app

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
```

Comment 27

[Alexandre Poirot [:ochameau]]

Attachment: Add test for app `redirects` manifest feature.

https://tbpl.mozilla.org/?tree=Try&rev=736c69ef34dd

Comment 28

[Alexandre Poirot [:ochameau]]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Gonna hold off on landing this until the test has r+. Please put checkin-needed back on when ready.

Comment 30

[[:fabrice] Fabrice Desré]

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 '='

Comment 31

[Alexandre Poirot [:ochameau]]

Attachment: Add test for app `redirects` manifest feature, nit addressed.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/a28690ed91ef
https://hg.mozilla.org/projects/birch/rev/b22b41460ba3

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a28690ed91ef
https://hg.mozilla.org/mozilla-central/rev/b22b41460ba3

Comment 34

[Donovan Preston [:fzzzy]]

Can this be uplifted to 1.1? It's leo+ and the patch is r+

Comment 35

[Alex Keybl [:akeybl]]

Removing NPOTB so that this gets uplifted. comment 25 was to keep it out of our queries.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Needs a branch-specific patch for uplift.

Comment 37

[Alexandre Poirot [:ochameau]]

Attachment: b2g18 - Ensure setting redirects property on app object in the actor

Here is the patch for b2g18. Unfortunately, tests depends on another bug that hasn't been uplifted (bug 844227).

Comment 38

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/68fb0a2e0114