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

RESOLVED FIXED in Firefox 45

Status

()

P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: stomlinson, Assigned: stomlinson)

Tracking

45 Branch
Firefox 46
Points:
---
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox45 fixed, firefox46 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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
(Assignee)

Comment 1

3 years ago
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.
(Assignee)

Updated

3 years ago
See Also: → bug 969474
(Assignee)

Updated

3 years ago
Assignee: nobody → stomlinson
(Assignee)

Comment 2

3 years ago
Created attachment 8700001 [details] [diff] [review]
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.
Attachment #8700001 - Flags: feedback?(markh)
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...
(Assignee)

Comment 4

3 years ago
> 
> 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
(Assignee)

Comment 5

3 years ago
(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
(Assignee)

Comment 6

3 years ago
Created attachment 8700077 [details] [diff] [review]
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?
Attachment #8700001 - Attachment is obsolete: true
Attachment #8700001 - Flags: feedback?(markh)
Attachment #8700077 - Flags: feedback?(markh)
Flags: firefox-backlog+
Priority: -- → P1
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
Attachment #8700077 - Flags: feedback?(markh) → review+
(Assignee)

Comment 8

3 years ago
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?
Flags: needinfo?(markh)

Updated

3 years ago
Flags: needinfo?(markh)
Keywords: checkin-needed

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9c4636421d71
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
(Assignee)

Comment 12

3 years ago
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.
Flags: needinfo?(ryanvm)
Flags: needinfo?(markh)
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.
Flags: needinfo?(ryanvm)
(Assignee)

Updated

3 years ago
Attachment #8700077 - Flags: approval-mozilla-aurora?
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
Flags: needinfo?(markh)
status-firefox45: --- → affected
Attachment #8700077 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 15

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/2169ff99d147
status-firefox45: affected → fixed
(Assignee)

Comment 16

3 years ago
Thanks :sylvestre, :Tomcat, and :markh!

Comment 17

3 years ago
[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
You need to log in before you can comment on or make changes to this bug.