Closed Bug 1305204 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Version:
50 Branch
Platform:
x86_64
Windows 8.1
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox49 --- unaffected
firefox50 + fixed
firefox51 + fixed
firefox52 + verified

People

(Reporter: kjb1611mikeiv, Assigned: valentin)

References

(
URL
)

Details

(Keywords: regression, site-compat, Whiteboard: [necko-active])

Attachments

(5 files)

test
432 bytes, text/html
Details
v2, with display: none
472 bytes, text/html
Details
Bug 1305204 - (Part 1) Backout bug 1275746 a=backout
58 bytes, text/x-review-board-request
Details
Bug 1305204 - (Part 2) Backout bug 1275746 a=backout
58 bytes, text/x-review-board-request
Details
Bug 1305204 - (Part 3) Backout bug 1275746 a=backout
58 bytes, text/x-review-board-request
ritu
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta+
Details 
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.
URL: http://webmail.earthlink.net
OS: Unspecified → Windows 8.1
Hardware: Unspecified → x86_64
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
Maybe a regression in FF50+. Do you know if we can test https://webmail.earthlink.net/ with a guest account?
Flags: needinfo?(kjb1611mikeiv)
Sure. Let me know when you are done with the account and I will delete it.

Address: bugzillatest@mindspring.com
Password: Pa55w0rd (Cap "P", lower "a", "fivefive", lower "w", "zero", lower "rd"

http://webmail.earthlink.net
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.
Blocks: 1275746
Status: UNCONFIRMED → NEW
status-firefox49: --- → unaffected
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
Component: Untriaged → Networking
Ever confirmed: true
Flags: needinfo?(kjb1611mikeiv) → needinfo?(valentin.gosu)
Keywords: regression
Product: Firefox → Core
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.
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.
Flags: needinfo?(valentin.gosu)
Keywords: site-compat
(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?
Flags: needinfo?(valentin.gosu)
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.
Flags: needinfo?(valentin.gosu)
Tracked for 50 since it seems to be a new regression.
tracking-firefox50: ? → +
(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?
Flags: needinfo?(amarchesini)
Track 51+ as regression.
tracking-firefox51: ? → +
Assignee: nobody → valentin.gosu
Whiteboard: [necko-active]
Any opinions on comment 10?
Flags: needinfo?(bugs)
Valentin, do you have some minimal testcases for this?
Flags: needinfo?(bugs) → needinfo?(valentin.gosu)
Attached file test — 
When using FF or Chrome I get similar 'Please enter a URL' message when clicking reportValidity
I mean when value is http://
Attached file v2, with display: none — 
This shows the message isn't shown in Chrome with display: none;
Same with visibility: hidden
Note, the spec lets UAs to do basically whatever.
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?
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(gijskruitbosch+bugs)
Gijs pointed out that it isn't still clear why the page breaks even if the popup is shown.
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
(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)
Flags: needinfo?(gijskruitbosch+bugs)
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.
I'm not sure the situation with the Presentation API. S-C?
Flags: needinfo?(schien)
Tracking 52+, especially if we are considering backing out patches.
tracking-firefox52: ? → +
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?
Flags: needinfo?(schien) → needinfo?(valentin.gosu)
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.
(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.
(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.
(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.
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
Hi Mike,
could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(kjb1611mikeiv)
(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.
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).
(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.
Flags: needinfo?(valentin.gosu)
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.
Attachment #8804060 - Flags: approval-mozilla-beta?
Attachment #8804060 - Flags: approval-mozilla-aurora?
(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.
Flags: needinfo?(kjb1611mikeiv)
makes sense to uplift this backout to 50 to me.
(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.
(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 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+
Attachment #8804060 - Flags: approval-mozilla-beta?
Attachment #8804060 - Flags: approval-mozilla-beta+
Attachment #8804060 - Flags: approval-mozilla-aurora?
Attachment #8804060 - Flags: approval-mozilla-aurora+
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
Blocks: 1319078
See Also: → 1373327
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: