1305204 - When trying to send/reply/forward an e-mail on webmail.earthlink.net I get a message "Please enter a URL"

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Mike]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20160920155715

Steps to reproduce:

When logged into webmail.earthlink.net I create a message or hit reply to a message and it loads a page to create and send a message. I put in an address of the recipient, a subject, and type a message. Then I hit the "Send" button.


Actual results:

When I hit the "Send" box, all the boxes grey out and a pop-down/quote box comes down from the "Back" (left facing arrow) next to the address bar that says "Please enter a URL"


Expected results:

It should send the message and reload the previous screen with a message across the top that it successfully sent it.

Comment 1

[Mike]

I have tried it on IE and it works there. I tried it in Firefox Safe Mode and it did the above thing. I also did a refresh of Firefox and it did the same. This has only been since the 50.0b1 and did not happen (to me) on 49 or below. I have not tried a complete reinstall, as it seems to be a bug and not an update issue at this point.

Here is a picture of what is happening when hitting the "Send" button:
http://technicaldifference.net/mikeiv/webmailerror.png

Comment 2

[Loic]

Maybe a regression in FF50+. Do you know if we can test https://webmail.earthlink.net/ with a guest account?

Comment 3

[Mike]

Sure. Let me know when you are done with the account and I will delete it.

Address: [e-mail]
Password: Pa55w0rd (Cap "P", lower "a", "fivefive", lower "w", "zero", lower "rd"

http://webmail.earthlink.net

Comment 4

[Loic]

Reg range: 
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a08e5896b83094df265f826091765d08a6e28b60&tochange=b31c2afa920a8f74ddddf0dc8d3eed7fef4ce795

Valentin, can you check this issue when sending an email from the webmail Earthlink after your patches in bug 1275746.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Those patches ensure that constructed URLs have a hostname.
It is possible that we are not more strict than other browsers, or that the web page is creating different URLs based on the user agent. So far I wasn't able to find what the offending URL is. I'll keep looking.

Comment 6

[Valentin Gosu [:valentin] (he/him)]

It seems that the message comes from our own form validation logic, not the web app:
http://searchfox.org/mozilla-central/source/dom/locales/en-US/chrome/dom/dom.properties#35

I've looked around the code for the web mail client, and found this:

<input id="linkInputURL" value="http://" size="50" type="url">

The problem is, that http:// is not a valid URL, and will fail the form validation.

Comment 7

[Dragana Damjanovic [:dragana]]

(In reply to Valentin Gosu [:valentin] from comment #6)
> It seems that the message comes from our own form validation logic, not the
> web app:
> http://searchfox.org/mozilla-central/source/dom/locales/en-US/chrome/dom/dom.
> properties#35
> 
> I've looked around the code for the web mail client, and found this:
> 
> <input id="linkInputURL" value="http://" size="50" type="url">
> 
> The problem is, that http:// is not a valid URL, and will fail the form
> validation.

Do other browser get a different url or the same and they are just less strict? In later case we should revert our change?

Comment 8

[Valentin Gosu [:valentin] (he/him)]

For some reason other browsers don't seem to validate that form input. I changed its value to &^%$ and it still worked in chrome.
I don't know if that's because it's hidden, or for some other reason.

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 50 since it seems to be a new regression.

Comment 10

[Valentin Gosu [:valentin] (he/him)]

(In reply to Valentin Gosu [:valentin] from comment #8)
> For some reason other browsers don't seem to validate that form input. I
> changed its value to &^%$ and it still worked in chrome.
> I don't know if that's because it's hidden, or for some other reason.

NI :baku who seems to have some contributions to HTMLFormElement.

I tested with Chrome, and it seems it also fails to submit a form that contains
<input type="url" value="http://">
I don't know the reason Chrome doesn't validate the URL input in the web client, but it might have something to do with its visibility. Is there anything in the spec that would suggest that? Or is it a fluke that all other browsers don't validate that input?

Comment 11

[Gerry Chang [:gchang]]

Track 51+ as regression.

Comment 12

[Valentin Gosu [:valentin] (he/him)]

Any opinions on comment 10?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Valentin, do you have some minimal testcases for this?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: test

When using FF or Chrome I get similar 'Please enter a URL' message when clicking reportValidity

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

I mean when value is http://

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2, with display: none

This shows the message isn't shown in Chrome with display: none;

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Same with visibility: hidden

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Note, the spec lets UAs to do basically whatever.

Comment 19

[Valentin Gosu [:valentin] (he/him)]

Thanks for the investigating, Olli. Is there anything we can we do here?
Do we want to change our behaviour, or should we just push the website operator to fix their code?

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

I think we should fix our behavior.
The relevant code is around http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/browser/modules/FormSubmitObserver.jsm#98
So somewhere there one should check at least if display is "none" or visibility "hidden".

But it isn't clear to me whether we should backout the regressing patch from FF50. 
That is of course the default action we should take, but is modifying FormSubmitObserver safe enough this late in beta?
Gijs might have an opinion.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Gijs pointed out that it isn't still clear why the page breaks even if the popup is shown.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Based on blink source code, they seem to use focusability as a check whether to show the popup.
https://chromium.googlesource.com/chromium/src/+blame/master/third_party/WebKit/Source/core/html/HTMLFormElement.cpp#240

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Olli Pettay [:smaug] from comment #21)
> Gijs pointed out that it isn't still clear why the page breaks even if the
> popup is shown.

Whether or not the popup is shown doesn't seem to affect here.
If I just modify FormSubmitObserver.jsm to return early, the issue still there.
(this should prevent also trying to focus some element)

Comment 24

[Valentin Gosu [:valentin] (he/him)]

I'm looking into backing out the patches. The backouts apply rather well, but I do have some odd test failures that don't happen locally:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c4b4915e73a&selectedJob=29405828
This makes me worry about potential regressions.

Comment 25

[Andrew Overholt [:overholt]]

I'm not sure the situation with the Presentation API. S-C?

Comment 26

[Marcia Knous [:marcia]]

Tracking 52+, especially if we are considering backing out patches.

Comment 27

[Andrew Overholt [:overholt]]

I spoke with S-C via email and he said "I think the modification is not related to the intermittent failure of presentation api tests. Feel free to try land it on inbound."

Valentin, do you have other concerns about potential regressions?

Comment 28

[:Gijs (he/him)]

I noted on IRC to Olli at the time that the "problem" seems to be that we validate when JS calls form.submit() and Chrome does not. No idea what the spec says on that matter, but could we align on that? AIUI changing the URL parsing behaviour will affect many other things, whereas the current behaviour (with the patch in) as far as validating URIs is concerned does match Chrome's - it's just that we use the result differently.

Comment 29

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch (out til 26th, no reviews) from comment #28)
> I noted on IRC to Olli at the time that the "problem" seems to be that we
> validate when JS calls form.submit() and Chrome does not. No idea what the
> spec says on that matter, but could we align on that? AIUI changing the URL
> parsing behaviour will affect many other things, whereas the current
> behaviour (with the patch in) as far as validating URIs is concerned does
> match Chrome's - it's just that we use the result differently.

Specifically, see this testcase: http://jsbin.com/kicokihaju/1/edit?html,output

which submits in Chrome but doesn't in Firefox.

Comment 30

[Valentin Gosu [:valentin] (he/him)]

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

Comment 31

[Valentin Gosu [:valentin] (he/him)]

(In reply to Andrew Overholt [:overholt] from comment #27)
> I spoke with S-C via email and he said "I think the modification is not
> related to the intermittent failure of presentation api tests. Feel free to
> try land it on inbound."

Actually, I just tracked down the issue, and it's an issue with the tests.
in test_presentation_dc_sender.html::testConstructRequestError() we do request = new PresentationRequest("\\\\\\"); which we expect to throw.
With the backout it doesn't throw as the URI resolves to resolves to http:///

> Valentin, do you have other concerns about potential regressions?

I am not too concerned about regressions, we had this URL parsing behaviour for a very long time.
But I would like to land it again as soon as possible, once we fix the form validation bug.
I'll land on inbound as soon as the try run finishes.

Comment 32

[Valentin Gosu [:valentin] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe38624aa14f6f1769f5d1e049e97ee89e119e34
Bug 1305204 - (Part 1) Backout bug 1275746 a=backout

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba408780dbcd994e10168394ede15795e8e5587b
Bug 1305204 - (Part 2) Backout bug 1275746 a=backout

https://hg.mozilla.org/integration/mozilla-inbound/rev/503de2e3054d20d8cc5825379c8d770c96968b7e
Bug 1305204 - (Part 3) Backout bug 1275746 a=backout

https://hg.mozilla.org/integration/mozilla-inbound/rev/a89de39a358784f56b19dfb3bfd32727ab102d42
Bug 1305204 - (Part 4) Comment out test with pseudo-bad URL r=me

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fe38624aa14f
https://hg.mozilla.org/mozilla-central/rev/ba408780dbcd
https://hg.mozilla.org/mozilla-central/rev/503de2e3054d
https://hg.mozilla.org/mozilla-central/rev/a89de39a3587

Comment 34

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Valentin Gosu [:valentin] from comment #31)
> (In reply to Andrew Overholt [:overholt] from comment #27)
> > I spoke with S-C via email and he said "I think the modification is not
> > related to the intermittent failure of presentation api tests. Feel free to
> > try land it on inbound."
> 
> Actually, I just tracked down the issue, and it's an issue with the tests.
> in test_presentation_dc_sender.html::testConstructRequestError() we do
> request = new PresentationRequest("\\\\\\"); which we expect to throw.
> With the backout it doesn't throw as the URI resolves to resolves to http:///
> 
> > Valentin, do you have other concerns about potential regressions?
> 
> I am not too concerned about regressions, we had this URL parsing behaviour
> for a very long time.
> But I would like to land it again as soon as possible, once we fix the form
> validation bug.
> I'll land on inbound as soon as the try run finishes.

Thanks @valentin for figuring out the relationship. There is a similar test case in web platform test, I can remove the test step from mochitest to avoid duplication.

Comment 35

[Mike]

Now that this issue is marked fixed as of version 52, is it safe for me to remove the email address? Or at least change the password to something more secure and it can then be given if needed for any future tests upon request? Or will it still be currently needed for an amount of time to come?

Thanks, Mike

Comment 36

[Gerry Chang [:gchang]]

Hi Mike,
could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

(In reply to :Gijs Kruitbosch (out til 26th, no reviews) from comment #28)
> I noted on IRC to Olli at the time that the "problem" seems to be that we
> validate when JS calls form.submit() and Chrome does not.

Oh, you changed the testcase now (since I couldn't reproduce the difference before, as I said on IRC). JS calls to form.submit() should and does not trigger validation in either browser().
It is that apparently if submit button tries to submit first in onclick, we re-trigger submit during default handling in such way that validation is done.

Changing this in beta is way too risky.

Comment 38

[Andrew Overholt [:overholt]]

Once we've verified, we'll need to uplift the backouts to beta (I confirmed with smaug that's what he meant in comment 37).

Comment 39

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1305204 - (Part 1) Backout bug 1275746 a=backout

Review commit: https://reviewboard.mozilla.org/r/88196/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/88196/

Comment 40

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1305204 - (Part 2) Backout bug 1275746 a=backout

Review commit: https://reviewboard.mozilla.org/r/88198/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/88198/

Comment 41

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1305204 - (Part 3) Backout bug 1275746 a=backout

Review commit: https://reviewboard.mozilla.org/r/88200/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/88200/

Comment 42

[Valentin Gosu [:valentin] (he/him)]

(In reply to Gerry Chang [:gchang] from comment #36)
> Hi Mike,
> could you please verify this issue is fixed as expected on a latest Nightly
> build? Thanks!

Also, although I don't completely understand what it's used for, I think the webmail code at earthlink should also be changed. 'http://' is not really a valid URL, and even though it led to finding this web-compatibility issue, it still feels quite hacky to me.

Comment 43

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8804060 [details]
Bug 1305204 - (Part 3) Backout bug 1275746 a=backout

Approval Request Comment
[Feature/regressing bug #]: bug 1275746 - it actually exposed a preexisting bug.
[User impact if declined]: Triggers form validation bug on webmail.earthlink.net
[Describe test coverage new/current, TreeHerder]: Reverts to previous behaviour. Unit tests have been updated.
[Risks and why]: Low risk. It reverts to previous behaviour.
[String/UUID change made/needed]: None.

Note: landed patches can be uplifted to aurora. Reviewboard patches have been rebased for beta.

Comment 44

[Mike]

(In reply to Gerry Chang [:gchang] from comment #36)
> Hi Mike,
> could you please verify this issue is fixed as expected on a latest Nightly
> build? Thanks!

Test verified on nightly 52.0a1 (2016-10-24) (64-bit)
The test e-mail was sent successfully and the behaviour of the original bug report does not happen.

Comment 45

[Patrick McManus [:mcmanus]]

makes sense to uplift this backout to 50 to me.

Comment 46

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] (reviewing overload, queue closed for a day or two) from comment #37)
> (In reply to :Gijs Kruitbosch (out til 26th, no reviews) from comment #28)
> > I noted on IRC to Olli at the time that the "problem" seems to be that we
> > validate when JS calls form.submit() and Chrome does not.
> 
> Oh, you changed the testcase now (since I couldn't reproduce the difference
> before, as I said on IRC). JS calls to form.submit() should and does not
> trigger validation in either browser().

Yes, sorry. When I looked back at IRC logs I found your message that said Firefox and Chrome behaved identically - I must have missed this at the time. I poked at it more and found the difference in the new testcase. Apologies, I should have clarified this in my comment.

> It is that apparently if submit button tries to submit first in onclick, we
> re-trigger submit during default handling in such way that validation is
> done.
> 
> Changing this in beta is way too risky.

Makes sense.

Comment 47

[Mike]

(In reply to Valentin Gosu [:valentin] from comment #42)
> Also, although I don't completely understand what it's used for, I think the
> webmail code at earthlink should also be changed. 'http://' is not really a
> valid URL, and even though it led to finding this web-compatibility issue,
> it still feels quite hacky to me.

I sent a "Customer Support" e-mail to them about this citing this bugzilla progress page. I do not know for sure it will get to the right place or people needed to make any changes or choices about this. I am not sure if there is a process whereby Mozilla can make official contact with Earthlink to make contact with such a person and see this change made on that end. If there is, please have someone make such a contact. Especially since Firefox may not be the only browser to ever see this issue and their support staff will not know how to fix this or give a proper suggestion on how to avoid it.

Comment 48

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8804060 [details]
Bug 1305204 - (Part 3) Backout bug 1275746 a=backout

Patch backout was verified on Nightly and this seems like a new regression in 50, Aurora51+, Beta50+

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0ab4ccecf1f8

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1fb1c935f887

Comment 51

[Mike]

Changing e-mail password now but not deleting account. If access is needed for testing in the future, contact me and I will change the password and let you know what it is again. - Mike