Closed Bug 761603 Opened 12 years ago Closed 11 years ago

UTF-8 handling is broken in Mozmill 2.0

Categories

(Testing Graveyard :: Mozmill, defect)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: whimboo, Assigned: whimboo)

References

()

Details

(Keywords: dataloss, regression, Whiteboard: [mozmill-2.0+][ateamtrack: p=mozmill q=2013q3 m=1])

Attachments

(1 file)

The handling of UTF-8 strings is broken again on master. Not sure when it has been regressed but it is working fine with Mozmill 1.5.12.

$ mozmill -b /Applications/Firefox/Nightly.app/ -t elementslib/utf-8.js 
TEST-START | elementslib/utf-8.js | setupModule
TEST-START | elementslib/utf-8.js | test
INFO | Step Pass: {"function": "Controller.open()"}
INFO | Step Pass: {"function": "controller.waitFor()"}
INFO | Step Pass: {"function": "controller.waitForPageLoad()"}
INFO | Step Pass: {"function": "MozMillElement.keypress()"}
INFO | Step Pass: {"function": "MozMillElement.keypress()"}
INFO | Step Pass: {"function": "MozMillElement.keypress()"}
INFO | Step Pass: {"function": "MozMillElement.keypress()"}
INFO | Step Pass: {"function": "MozMillElement.keypress()"}
INFO | Step Pass: {"function": "MozMillElement.keypress()"}
INFO | Step Pass: {"function": "MozMillElement.keypress()"}
INFO | Step Pass: {"function": "MozMillElement.keypress()"}
INFO | Step Pass: {"function": "MozMillElement.keypress()"}
INFO | Step Pass: {"function": "MozMillElement.keypress()"}
INFO | Step Pass: {"function": "MozMillElement.keypress()"}
RESULTS | Passed: 0
RESULTS | Failed: 0
RESULTS | Skipped: 0

Lets wait until bug 760418 has been checked-in which fixes all the tests and also brings back this failure.
The test I'm referring to is:
https://github.com/mozautomation/mozmill/blob/master/mutt/mutt/tests/js/elementslib/utf-8.js

It should better be moved under tests/js/frame. Not sure why I have put it under elementslib.
Regressing check-in found after quite a long time of testing. Now it's up to you to fix it. :)


451b53da80eac72445a73aafdfd3d85eac2b7918 is the first bad commit
commit 451b53da80eac72445a73aafdfd3d85eac2b7918
Author: unknown <mozilla@.(none)>
Date:   Thu May 12 13:57:56 2011 -0700

    Bug 656745 fix cannot unload profile on windows error messages r=jhammel

:040000 040000 cb143f65608029bf530619f076ee0811e0545153 4d1a91e205ac176f11d6eebb05017b6fcbde707e M	mozmill
Whiteboard: [mozmill-2.0?] → [mozmill-2.0+]
Whiteboard: [mozmill-2.0+] → [mozmill-2.1+]
Also see bug 793764 for additional failures when submitting special characters.
Whiteboard: [mozmill-2.1+] → [mozmill-2.1+][mentor=whimboo][lang=py][lang=js]
Blocks: 889817
We might have to re-consider this for mozmill 2.0. See bug 877100 for a set of current failures due to unicode characters e.g. in the name and description of the default theme.

I will take this.
Assignee: nobody → hskupin
Blocks: 877100
Status: NEW → ASSIGNED
Whiteboard: [mozmill-2.1+][mentor=whimboo][lang=py][lang=js] → [mozmill-2.0?]
No longer blocks: 889817
Attached patch Patch v1Splinter Review
Actually I have no idea why we ever decided to send data explicitly in utf-8 encoding through jsbridge. It might have been required at that time, but as of today it is not! We only have to create a JSON string and thats all. Everything gets correctly transferred both ways. The test attached demonstrates that and passes on all platforms. Further I have also run tests for all available locales for Firefox 22.0 to ensure we do not regress something. All of those builds look fine and the test passes.

Jeff, can you remember why we did that? If not, does that look good for you?

Andrei, would you mind to test this patch on Windows XP? Just use an en-US build for the mutt tests, but also test with a csb build if the addons are correctly listed in the report.
Attachment #780639 - Flags: review?(dave.hunt)
Attachment #780639 - Flags: feedback?(jhammel)
Attachment #780639 - Flags: feedback?(andrei.eftimie)
This bug should absolutely be a blocker for Mozmill 2.0.
Whiteboard: [mozmill-2.0?] → [mozmill-2.0+]
Severity: normal → critical
Keywords: dataloss
(In reply to Henrik Skupin (:whimboo) from comment #5)
> Created attachment 780639 [details] [diff] [review]
> Patch v1
> 
> Actually I have no idea why we ever decided to send data explicitly in utf-8
> encoding through jsbridge. It might have been required at that time, but as
> of today it is not! We only have to create a JSON string and thats all.
> Everything gets correctly transferred both ways. The test attached
> demonstrates that and passes on all platforms. Further I have also run tests
> for all available locales for Firefox 22.0 to ensure we do not regress
> something. All of those builds look fine and the test passes.
> 
> Jeff, can you remember why we did that? If not, does that look good for you?

TBH, I don't recall why we did what we did....only that it was deemed necessary at the time. Nor do I think it is worth a spelunking expedition to find out...if what is here or similar works...its good by me.
Comment on attachment 780639 [details] [diff] [review]
Patch v1

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

As long as this works....lgtm!
Attachment #780639 - Flags: feedback?(jhammel) → feedback+
Comment on attachment 780639 [details] [diff] [review]
Patch v1

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

Looks good. According to github the utf-8 was introduced in https://github.com/mozilla/mozmill/commit/715bb4468f9671903ed3c7641101b6232bef80a0 which mentions "use NSPR sockets in jsbridge to get around offline disabling localhost" we should check that this change hasn't regressed that fix.
Attachment #780639 - Flags: review?(dave.hunt) → review+
(In reply to Dave Hunt (:davehunt) from comment #9)
> Looks good. According to github the utf-8 was introduced in
> https://github.com/mozilla/mozmill/commit/
> 715bb4468f9671903ed3c7641101b6232bef80a0 which mentions "use NSPR sockets in
> jsbridge to get around offline disabling localhost" we should check that
> this change hasn't regressed that fix.

It was not that well tested at this time I believe. I think that this hasn't been checked with all the locales, as what I did yesterday. I haven't noticed any single issue with the patch been applied. The only thing I want to know before landing is that it will also work on WinXP.
Comment on attachment 780639 [details] [diff] [review]
Patch v1

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

This fixes mutt failures on WinXP with exotic locales.

This is what I've tested (and by miserably i mean about 20 failures)

master
======
en-US: pass
fr: fail miserably
csb: fail miserably

patch applied
=============
en-US: pass
fr: pass
csb: pass
Attachment #780639 - Flags: feedback?(andrei.eftimie) → feedback+
Landed as:
https://github.com/mozilla/mozmill/commit/50a71593110e0ab1d3a1e5adf12becc21b4015e8
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [mozmill-2.0+] → [mozmill-2.0+][ateamtrack: p=mozmill q=2013q3 m=1]
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.