1233668 - "back" link does not work from "choose what to sync" in Firefox Accounts when embedded in about:accounts

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Shane Tomlinson [:stomlinson]]

Cross-filed from https://github.com/mozilla/fxa-content-server/issues/3329

The "back" link on the Firefox Accounts "Choose what to Sync" page does not work when FxA is embedded in about:accounts.

The root cause is a Firefox bug. about:accounts adds the FxA iframe in aboutaccounts.xhtml without setting an initial `src` attribute. The initial `src` attribute is set to `about:blank` by default. Fx then sets the src attribute without adding the first page to history [1] using:

> webNav.loadURI(url, Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_HISTORY, null, null, null);

When the "back" link is clicked, history goes back to `about:blank`, which ends up in a SecurityError being thrown.

Introduced by the patch for https://bugzilla.mozilla.org/show_bug.cgi?id=969474


[1] - https://hg.mozilla.org/mozilla-central/annotate/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/browser/base/content/aboutaccounts/aboutaccounts.js#l133

Comment 1

[Shane Tomlinson [:stomlinson]]

I believe the proper solution here is to add the iframe to the DOM using JavaScript after the initial `src` attribute is set, without bypassing history. I believe this should avoid an initial history entry of `about:blank` and the behavior outlined in 969474.

Comment 2

[Shane Tomlinson [:stomlinson]]

Attachment: Add the FxA iframe to the DOM once the initial URL is known - fixes back button behavior.

Mark, this is my proposed patch, without tests. The tests are doing my head in. I actually have all the tests passing, but want to dig into why some tests that seem like they should fail are passing, so I want to make sure the approach is sane before spending any more time on this.

The basic idea - I believe the original problem of the back button being enabled occurred because the FxA iframe was in the DOM w/o a src attribute. Fx used "about:blank" for the default src. Afterwards, the actual src value was set, so history had 2 entries. The user could click "back" but nothing happened. This caused us problems because if the user when to choose what to sync and then hit the back "link" or back button, history sent the user back to "about:blank" which causes a cascading failure.

Initial behavior seems to be the same between your master and the current patch, loading about:accounts in a new tab has the browser's back button disabled. Loading about:accounts via about:preferences#sync and then clicking "back" goes back to about:preferences#sync. CWTS works correctly.

If the approach is reasonable, I'll finish the tests over the weekend or Monday.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8700001 [details] [diff] [review]
Add the FxA iframe to the DOM once the initial URL is known - fixes back button behavior.

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

I wonder if we can still manipulate history and get the result we need? Eg, replace the about:blank entry? I feel like we are still missing something simple here...

Comment 4

[Shane Tomlinson [:stomlinson]]

> 
> I wonder if we can still manipulate history and get the result we need? Eg,
> replace the about:blank entry? I feel like we are still missing something
> simple here...

Thanks Mark,

I have a content server only workaround in [1].

I don't mind a content server only patch, but it doesn't work around [2], whereas a this browser patch does. Perhaps, like you point out, the browser calling history.replaceState itself would be a simple fix.

[1] - https://github.com/mozilla/fxa-content-server/pull/3333
[2] - https://github.com/mozilla/fxa-content-server/issues/3335

Comment 5

[Shane Tomlinson [:stomlinson]]

(In reply to Mark Hammond [:markh] from comment #3)
> Comment on attachment 8700001 [details] [diff] [review]
> Add the FxA iframe to the DOM once the initial URL is known - fixes back
> button behavior.
> 
> Review of attachment 8700001 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I wonder if we can still manipulate history and get the result we need? Eg,
> replace the about:blank entry? I feel like we are still missing something
> simple here...

The answer is so simple! Replace `LOAD_FLAGS_BYPASS_HISTORY` with `LOAD_FLAGS_REPLACE_HISTORY` on [1].

Thanks Mark! New patch incoming tonight or Monday.

[1] - https://hg.mozilla.org/mozilla-central/annotate/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/browser/base/content/aboutaccounts/aboutaccounts.js#l133

Comment 6

[Shane Tomlinson [:stomlinson]]

Attachment: Use LOAD_FLAGS_REPLACE_HISTORY to replace the about:blank history entry.

You were correct Mark, the patch could be *vastly* simplified. Now, how would you go about testing this?

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8700077 [details] [diff] [review]
Use LOAD_FLAGS_REPLACE_HISTORY to replace the about:blank history entry.

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

TBH I don't think it's worth the pain of trying to test this given the impact is so minimal

Comment 8

[Shane Tomlinson [:stomlinson]]

Thanks Mark,
Two more requests: I have no commit access, can you land it for me? And assuming everything works as expected, can we we get this uplifted into Fx 45 so the fix goes out with the web based Choose what to Sync?

Comment 9

[Shane Tomlinson [:stomlinson]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50ba4cf06db3

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9c4636421d71

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c4636421d71

Comment 12

[Shane Tomlinson [:stomlinson]]

Thanks both Mark and Ryan. Can this patch be uplifted this to Fx 45 so the fix goes out when the Choose what to Sync on the Web feature is enabled? Otherwise we are going to enable Choose what to Sync and it'll have a broken back button.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

You would need to go through the regular approval process if you want it to be uplifted to Aurora. Start by setting approval-mozilla-aurora? on the patch and answering the questions it asks.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8700077 [details] [diff] [review]
Use LOAD_FLAGS_REPLACE_HISTORY to replace the about:blank history entry.

Approval Request Comment
[Feature/regressing bug #]: FxA Sync
[User impact if declined]: The "Back" button will not work correctly when signing up for a Firefox account.
[Describe test coverage new/current, TreeHerder]: Existing tests pass
[Risks and why]: Very simple patch with low risk limited to about:accounts.
[String/UUID change made/needed]: None

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2169ff99d147

Comment 16

[Shane Tomlinson [:stomlinson]]

Thanks :sylvestre, :Tomcat, and :markh!

Comment 17

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS				Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected