1078718 - Incorrect URL screen results in "dead" tab

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect)

Description

[Nils Ohlmeier [:drno]]

After clicking on the green "Start" button on a invalid URL like this https://call.mozilla.com/#call/joL2Z0NlHw1 I get an error page "Oops! Sorry, this URL is not available. ...".
If I try to fix the incorrect call URL parameter from joL2Z0NlHw1 to joL2Z0NlHw0 (just examples), nothing happens. Manipulating the URL parameters and hitting enter seems to do nothing.
You can only:
- close the tab and open a new one
- or reload the tab and then fix the URL

BTW manipulating the call URL parameter on this error page and then hitting the reload icon in the URL bar, results in reloading the page with the new parameter?!

Comment 1

[Mark Banner (:standard8)]

Having talked to NiKo, we think the best thing to do for now would be to listen to the window's hashchange event, and when it fires, force a reload.

This saves us having from having to abort any states, and ensures we get a fresh set-up with the new token.

Comment 2

[Andrei Oprea [:andreio]]

Attachment: Force standalone app reload on hashchange event

Comment 3

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8502498 [details] [diff] [review]
Force standalone app reload on hashchange event

Review of attachment 8502498 [details] [diff] [review]:
-----------------------------------------------------------------

To make all this a bit more reusable, I'd rather see this split in two simple mixins: 

- a UrlHashChangeMixin allowing the component it's applied to define a onUrlHashChange method, which would be called every time the url hash changes;
- a DocumentLocation mixin, exposing the locationReload method

That'd allow us to define use them that way:

var Component = React.createClass({
  mixins: [UrlHashChangeMixin, DocumentLocationMixin],
  
  onUrlHashChange: function() { // called by UrlHashChangeMixin
    this.locationReload();      // provided by DocumentLocationMixin
  }
});

What do you think?

Comment 4

[Andrei Oprea [:andreio]]

Attachment: Force standalone app reload on hashchange event

Comment 5

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8503068 [details] [diff] [review]
Force standalone app reload on hashchange event

Review of attachment 8503068 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great.

::: browser/components/loop/content/shared/js/mixins.js
@@ +32,5 @@
> +   */
> +  var UrlHashChangeMixin = {
> +    _onUrlHashChange: function() {
> +      if (!this.onUrlHashChange) {
> +        throw new ReferenceError("onUrlHashChange callback is not defined");

Let's just fail silently.

Comment 6

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #5)
> ::: browser/components/loop/content/shared/js/mixins.js
> > +        throw new ReferenceError("onUrlHashChange callback is not defined");
> 
> Let's just fail silently.

Now I think about it, what we probably want here is to define propTypes for each mixins, and ask for the onUrlHashChange prop to be a required function.

Comment 7

[Maire Reavy [:mreavy]]

Niko -- Is this ready to land (with a little clean up)?

Comment 8

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

This can land with the comment addressed, yes. I'll update the patch.

Comment 9

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Force standalone app reload on hashchange event

Updated patch to address the previsou comment, and updated mixin tests to not use the real DOMWindow object.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8510902 [details] [diff] [review]
Force standalone app reload on hashchange event

Review of attachment 8510902 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. r=Standard8.

Note that we won't need this code for a long time, as bug 1046114 is changing the formats. However, its useful in the short term as it lets us fix the issue and not need to be dependent on when the configuration changes for the various servers are rolled out.

Comment 11

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/86a90b651868

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/86a90b651868

Comment 13

[u279076]

Nils, can you please verify this is fixed in tomorrow's Nightly?

Comment 14

[Maire Reavy [:mreavy]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #13)
> Nils, can you please verify this is fixed in tomorrow's Nightly?

Nils -- is this fixed for you?  (Thanks!)

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/88c62ec930b7

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8510902 [details] [diff] [review]
Force standalone app reload on hashchange event

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only

Comment 17

[Nils Ohlmeier [:drno]]

It is not fixed for me. I still get exactly the behavior as before on 36.0a1 (2014-11-13) and 35.0a2 (2014-11-13).

But if my understanding is correct this can't get magically fixed in Firefox as we are talking about URL's and content getting delivered through the Loop content server, or?
So I would need to either wait for the next production deployment which includes this fix, or ask the services guys for a deployment which includes this fix for test verification.

Comment 18

[Maire Reavy [:mreavy]]

Hi Nils, I'm sorry that I wasn't clearer.  This is standalone.  So you would need to test it against a dev server that has this fix -- or test it when the next standalone is deployed to prod (which is scheduled to be pushed out tomorrow).  At this point I'll ping you right after the new standalone is out.

Comment 19

[Nils Ohlmeier [:drno]]

I can test against dev if that helps and someone lets me know the URL for dev.

Comment 20

[Paul Silaghi, QA [:pauly]]

Verified fixed on the standalone page in FF 35b5, Chrome.