1135943 - Remove custom event handler in aboutaccounts

Status: RESOLVED DUPLICATE of bug 1383663

(Firefox :: Sync, defect, P3)

Description

[Zachary Carter [:zaach]]

The FxDesktop channel has been deprecated. We should eventually refactor about:accounts to use the new WebChannel interface instead.

Comment 1

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

Or the facility added in bug 1068087?

Comment 2

[Chris Karlof [:ckarlof]]

> Or the facility added in bug 1068087?

Yeah, wild. Whatever works.

Comment 3

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

I can't recall exactly what "The FxDesktop channel" means now, but in our triage we guessed it probably means the custom event handler in aboutaccounts, and I agree that we shouldn't be using a hand-rolled message manager here (it's relatively risky and high value), and also don't see why it can't be completely replaced with our webchannel messages.

Shane, this will obviously require some coordination with the FxA team - does this sound reasonable? If so, any thoughts on when/how we might be able to schedule it?

Comment 4

[Ryan Kelly [:rfkelly]]

> I can't recall exactly what "The FxDesktop channel" means now, but in our
> triage we guessed it probably means the custom event handler in aboutaccounts

That's my understanding, this is talking about the handling of "FirefoxAccountsCommand" events in about:accounts.

But as far as I can tell, about:accounts loads FxA with ?context=fx_desktop_v3, and the fx_desktop_v3 auth-broker uses webchannels exclusively.  So I wonder if this code is actuall dead already and just needs to be removed.  I don't see how the web content loaded by about:accounts could be sending "FirefoxAccountsCommand" events (but obviously Shane may have more perspective on this).

Comment 5

[Shane Tomlinson [:stomlinson]]

(In reply to Mark Hammond [:markh] from comment #3)
> I can't recall exactly what "The FxDesktop channel" means now, but in our
> triage we guessed it probably means the custom event handler in
> aboutaccounts, and I agree that we shouldn't be using a hand-rolled message
> manager here (it's relatively risky and high value), and also don't see why
> it can't be completely replaced with our webchannel messages.
> 
> Shane, this will obviously require some coordination with the FxA team -
> does this sound reasonable? If so, any thoughts on when/how we might be able
> to schedule it?

I actually have a (now very old) Firefox patch on my old laptop to rip out all of the FirefoxAccountsCommand code. IIRC, ripping out the code was the easy part, updating the tests was not, simply because I didn't know what I was doing.

But, :rfkelly is correct, about:accounts should open FxA with the ?context=fx_desktop_v3 query parameter,
which forces FxA to use WebChannel messages anyways. All of the functions in [1][2][3][4] return `fx_desktop_v3` as the context.

I can see if I still have that beginnings of a patch lying around on the old laptop. I might not, I blew away the mozilla-central directory to save space at one point, I'm unsure if that was before or after writing the patch. If I don't have the patch, you should be clear to remove all that old cruft whenever you want.


[1] - https://dxr.mozilla.org/mozilla-central/rev/845cc4dea57f6cc93f46810d24b1058b640c3b74/browser/base/content/aboutaccounts/aboutaccounts.js#359
[2] - https://dxr.mozilla.org/mozilla-central/rev/845cc4dea57f6cc93f46810d24b1058b640c3b74/browser/base/content/aboutaccounts/aboutaccounts.js#370
[3] - https://dxr.mozilla.org/mozilla-central/rev/845cc4dea57f6cc93f46810d24b1058b640c3b74/browser/base/content/aboutaccounts/aboutaccounts.js#381
[4] - https://dxr.mozilla.org/mozilla-central/rev/845cc4dea57f6cc93f46810d24b1058b640c3b74/browser/base/content/aboutaccounts/aboutaccounts.js#396

Comment 6

[Shane Tomlinson [:stomlinson]]

:markh, I looked for the above mentioned patch, nowhere to be found. Then I remembered something else - my patch was to DO AWAY WITH about:accounts all together. IIRC, about:accounts was created primarily as the one and only page that could receive the above referenced FirefoxAccountsCommand. FxA had to be embedded in about:accounts so it could send the FirefoxAccountsCommand and have them be listened to. This is why WebChannels were so great for us, we could send messages to the browser from any accounts.firefox.com tab.

My patch was to completely bypass/rip out about:accounts and any time the user clicked on sign in, or sign up, open the appropriate FxA page in a new tab directly.

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to Shane Tomlinson [:stomlinson] from comment #6)
> :markh, I looked for the above mentioned patch, nowhere to be found. Then I
> remembered something else - my patch was to DO AWAY WITH about:accounts all
> together. IIRC, about:accounts was created primarily as the one and only
> page that could receive the above referenced FirefoxAccountsCommand. FxA had
> to be embedded in about:accounts so it could send the FirefoxAccountsCommand
> and have them be listened to. This is why WebChannels were so great for us,
> we could send messages to the browser from any accounts.firefox.com tab.
> 
> My patch was to completely bypass/rip out about:accounts and any time the
> user clicked on sign in, or sign up, open the appropriate FxA page in a new
> tab directly.

Just for context, I recall on Android there were two reasons to use about:accounts rather than accounts.firefox.com in a tab:
1) vague security concerns: it was thought to be easier to _respond_ to incidents if we had a single special URL to worry about.  I think ckarlof cast the deciding vote here but my memory may fail me.
2) we wanted to show special UI -- a spinner and custom error messages if accounts.firefox.com can't be reached -- while the tab was loading.  This is important because we can open the browser directly to about:accounts on Android, and it looks really bad if the page never loads (due to connectivity issues).  I don't know if this is worth the effort to preserve.

Comment 8

[Shane Tomlinson [:stomlinson]]

(In reply to Nick Alexander :nalexander (leave until January 2017) from comment #7)

> Just for context, I recall on Android there were two reasons to use
> about:accounts rather than accounts.firefox.com in a tab:
> 1) vague security concerns: it was thought to be easier to _respond_ to
> incidents if we had a single special URL to worry about.  I think ckarlof
> cast the deciding vote here but my memory may fail me.
> 2) we wanted to show special UI -- a spinner and custom error messages if
> accounts.firefox.com can't be reached -- while the tab was loading.  This is
> important because we can open the browser directly to about:accounts on
> Android, and it looks really bad if the page never loads (due to
> connectivity issues).  I don't know if this is worth the effort to preserve.

Thanks for the context :nalexander, that's very useful to know. I imagine 
about:accounts provides a similar safety net on Desktop too, where 
connectivity is generally more reliable.

An additional data point is the firstrun flow on Desktop. A user that opens a new 
profile on Desktop is immediately displayed the firstrun page [1], which is itself
web content. The firstrun page loads FxA within an iframe. The firstrun page used 
to display a loading throbber overlaying the FxA iframe until FxA was fully loaded.
I no longer see the throbber, but it used to be it was displayed until a `loaded` 
message was sent from FxA to the firstrun page via a postMessage.

I am not sure how they handled if FxA never loaded, :jpetto?

If it's decided that about:accounts should stay around on desktop, then should the
settings page also load in about:accounts when a signed in user clicks on 
"Manage Account" from "about:preferences#sync"?

[1] - https://www.mozilla.org/en-US/firefox/51.0/firstrun/  <-- The version number changes with the Firefox version.

Comment 9

[Jonathan Petto [:jpetto]]

I don't think we've ever handled the case where FxA just doesn't respond. We have a timeout that will display the iframe after 400ms, regardless of any interaction between the page and the iframe. This ends up displaying whatever the iframe URL returns - the correct form, a 500 error, etc.

I just talked this over with agibson, and we're going to file a bug to better handle the case where the 'loaded' event is never received from the iframe.

FWIW, I think that spinner was either on the FxA side or possibly in a test variant. I did a quick search in the history and didn't find any evidence. (Though the firstrun page has been *heavily* iterated, so I could be wrong.)

Comment 10

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

(In reply to Shane Tomlinson [:stomlinson] from comment #8)
> Thanks for the context :nalexander, that's very useful to know. I imagine 
> about:accounts provides a similar safety net on Desktop too, where 
> connectivity is generally more reliable.

Yes, in theory we show the <div> at https://dxr.mozilla.org/mozilla-central/rev/8eaf154b385bbe0ff06155294ccf7962aa2d3324/browser/base/content/aboutaccounts/aboutaccounts.xhtml#73 on a network error.

I think nuking about:accounts is a good idea, but is likely to be more difficult for exactly this "what to do on no network?" issue, so I think we should take the low-road here and just nuke the custom event handler.

Comment 11

[ewhite7]

I'm a student at Seneca college learning open source, and I was hoping to work on this bug for my course. However i am a first timer and might need some help with locating the files

Comment 12

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

(In reply to ewhite7 from comment #11)
> I'm a student at Seneca college learning open source, and I was hoping to
> work on this bug for my course. However i am a first timer and might need
> some help with locating the files

Thanks for your interest in helping make Firefox great! The first step is to follow the guide at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction to learn how to build Firefox. Once you have done this you will have all of the files needed in a local checkout and it will be some of these files that will need to be changed. Please let us know in thus bug when you have got a Firefox build running locally and we'll give you some additional pointers for how to solve this bug.

Comment 13

[Edouard Oger [:eoger]]

Actually I don't think there's anything left to do since the work has been done in bug 1383663. Shall we dupe this bug?

Comment 14

[Lina Butler [:lina]]

This does look like a dupe of bug 1383663, and we're planning to eventually remove about:accounts in bug 1395460.