506760 - Mozmill tests with UTF-8 characters involved cannot be executed

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect, P1)

Description

[cmtalbert]

Attachment: testcase that demonstrates the bug

I'm not sure if this is an editor or a subsystem bug.  At the moment, it looks like an editor bug because if you have a UTF-8 character in your test it gets turned into an "&" when you save the test (which happens before running it).  If you run the test from the command line it should work because the subsystem should accept UTF-8 characters

TODO: Someone run this from the command line and see if we have a bigger problem here than just the editor not handling utf-8.

Steps
1. Load test in editor
2. Run it.
3. Watch it fail.

Expected
Editor can run this without any issues.

== Note ==
The minute you load this in the mozmill editor the test case is destroyed.  It will replace the ellipsis character with an & upon saving this test case (or running it). The character in mention is at line 126 of the test case. at the end of the lookup expression.  The label we are trying to match contains a UTF-8 ellipsis, and we need to do the same.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We never should try to compare labels. But similar things can happen in other areas of tests. So this should not happen. 

This is a reduced test we should look at.

var testUnicode = function() {
  controller = mozmill.getBrowserController();
  controller.window.alert("This isn\'t a web forgery…");
}

Comment 2

[Merike Sell (:merike)]

Ran a similar testcase for Thunderbird (UTF-8 file saved with external editor Kate) and 
ü -> ü
õ -> õ
ä -> ä

Used commandline Mozmill 1.3.

Comment 3

[Mikeal Rogers [:mikeal] (mrogers@)]

harthur has a branch up with a ton of UI rework, including integration of Bespin for the editor:

http://github.com/harthur/mozmill/tree/newui

I doubt we'll have these unicode issues with Bespin.

Comment 4

[Heather Arthur [:harth]]

yeah, seems fine with the Bespin, saving or running a file with those characters does not convert them. but Merike, you said this happened just using the command line Mozmill?

Comment 5

[Merike Sell (:merike)]

(In reply to comment #4)
> yeah, seems fine with the Bespin, saving or running a file with those
> characters does not convert them. but Merike, you said this happened just using
> the command line Mozmill?

Yes, that's what I see, both IDE and commandline break on utf-8 characters. With commandline I don't see them entered properly. Select call fails entirely.

Since this bug didn't mention if anyone had tried from commandline I noted that it didn't work for me. As you're saying that with a different editor it runs fine then I'm starting to doubt my system/setup.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

That sounds weird. Merike, does it happen for all text fields? I'm not able to reproduce such an issue from the command line. If you can reproduce it with Firefox it would be nice to have a short testcase for it.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Oh, and that should be another bug.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

After talking with Merike on IRC this is not only a problem with the IDE. We fail too when running such a test from the command line. Looks like we don't load test files as UTF-8.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This is getting more and more important. Looks like we should check what we can do here for 1.4.2.

Comment 10

[Heather Arthur [:harth]]

(In reply to comment #1)
> We never should try to compare labels. But similar things can happen in other
> areas of tests. So this should not happen. 
> 
> This is a reduced test we should look at.
> 
> var testUnicode = function() {
>   controller = mozmill.getBrowserController();
>   controller.window.alert("This isn\'t a web forgery…");
> }

If alert converts ellipses … to &, then it's not our fault. Is there a reduced testcase that doesn't involve alert?

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Heather, see bug 559814 for a testcase. The reason why that happens should be bug 377498 IMO.

Comment 12

[Heather Arthur [:harth]]

nm, alert('…'); from the error console shows the ellipses properlly

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

It's the way how we execute our js files. Also try something like that:

controller.window.alert("tanowiących");

The alert will work well in the error console but it fails when you run it with Mozmill. As given by bug 377498 the mozIJSSubScriptLoader only accepts ISO-8859-1 charsets and doesn't support UTF-8.

We make use if it here:
http://github.com/mikeal/mozmill/blob/master/mozmill/extension/resource/modules/frame.js#L52

Comment 14

[Heather Arthur [:harth]]

Thanks for pointing that out Henrik, I looked at a few other script loading options:

eval - doesn't support passing in a context.

evalInSandbox - can pass in a context but has too many security restrictions.

<script> tag - what window do we put it on? I couldn't get it to work on the hiddenDOMWindow, and it's asynchronous, so we'd have to change some code.

I think the unicode-encoding workaround mentioned in bug 377498 is our best bet. I have something working with this, but I need to clean it up.

Comment 15

[Heather Arthur [:harth]]

I have a fix for this in my encoding branch:

http://github.com/harthur/mozmill/compare/master...encoding

It converts the test to unicode, saves it to a temp file, and mozSubScriptLoader loads the temp file.

with this fix, the alert testcase now works, but this does not fix bug 559814.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #15)
> with this fix, the alert testcase now works, but this does not fix bug 559814.

Is that somehow related how we read those values from property files or dtd files? Not sure if we should explicitly use an UTF-8 specific function, if that is available? Or at least transform the values to UTF-8 once they have been read from the file.

Comment 17

[Jeff Hammel]

(In reply to comment #15)
> I have a fix for this in my encoding branch:
> 
> http://github.com/harthur/mozmill/compare/master...encoding
> 
> It converts the test to unicode, saves it to a temp file, and
> mozSubScriptLoader loads the temp file.
> 
> with this fix, the alert testcase now works, but this does not fix bug 559814.

This branch no longer exists.

Comment 18

[Jeff Hammel]

Attachment: mozmil unicode utilities

Comment 19

[Jeff Hammel]

Attachment: jsbridge unicode utilities

Comment 21

[Jeff Hammel]

(In reply to comment #20)
> *** Bug 580431 has been marked as a duplicate of this bug. ***

After IRC conversations, it looks like this the patches I put as bug 580431 actually belong with this bug.  I've moved them here and closed the previous as a duplicate.  The are probably part of harthur's no longer existant encoding branch. Comments from bug 580431 still apply.

Comment 22

[Jeff Hammel]

Attachment: update of jsbridge unicode utilities patch to today's master

i've updated the patch for jsbridge to cleanly apply to today's code.  However, it does not work:

<snip/>
 File "/home/jhammel/mozmill/src/mozmill/jsbridge/jsbridge/network.py", line 196, in run
    raise JSBridgeDisconnectError("Connection timed out")
jsbridge.network.JSBridgeDisconnectError: Connection timed out

I'm not sure if this patch is still necessary/a good idea

Comment 23

[Jeff Hammel]

(In reply to comment #14)
<snip/>
> I think the unicode-encoding workaround mentioned in bug 377498 is our best
> bet. I have something working with this, but I need to clean it up.

Which workaround do you speak of?  Is this the same as the work you did in the encoding branch below (presumedly captured in the mozmill patch attached to this bug)?

Comment 24

[Jeff Hammel]

(In reply to comment #15)
> I have a fix for this in my encoding branch:
> 
> http://github.com/harthur/mozmill/compare/master...encoding
> 
> It converts the test to unicode, saves it to a temp file, and
> mozSubScriptLoader loads the temp file.
> 
> with this fix, the alert testcase now works, but this does not fix bug 559814.

I'm a bit confused by this fix (I assume this is the patch https://bug506760.bugzilla.mozilla.org/attachment.cgi?id=459532 ?). If I understand, this will fix files that are natively iso-8859-1.  Do we care about this use case (and/or do I understand correctly)?  Related to the discussion on 580431 , I'm not sure if if we either care about or if it's a good idea to support non-unicode test files.  While this fix works for iso-8859-1, AFAIK the test files should be in UTF-8 anyway and we're probably not interested in supporting all the charsets (I don't think?).  And other than the case that the file is originally in iso-8859-1, I'm not sure what problem this really solves, unless Components.interfaces.nsIScriptableUnicodeConverter actually converts to escaped (ASCII) unicode vs real utf-8 files (I think the latter).  So, I'm not sure what problem this solves?

Comment 25

[Adrian Kalla [:adriank]]

(In reply to comment #22)
> Created attachment 459543 [details] [diff] [review]
> update of jsbridge unicode utilities patch to today's master
> 
> i've updated the patch for jsbridge to cleanly apply to today's code.  However,
> it does not work:
> 
> <snip/>
>  File "/home/jhammel/mozmill/src/mozmill/jsbridge/jsbridge/network.py", line
> 196, in run
>     raise JSBridgeDisconnectError("Connection timed out")
> jsbridge.network.JSBridgeDisconnectError: Connection timed out
> 
> I'm not sure if this patch is still necessary/a good idea

Jeff, the old patch still works and applies cleanly on today's code. Your updated patch doesn't work, because you added the last two lines in the wrong place - they have to go into the try of Session.prototype.onOutput .

The Heather's JSBridge patch alone fixes the most use cases of bug 559814.

Comment 26

[cmtalbert]

Attachment: The correct jsbridge patch

Verified that this patch to jsbridge will solve this bug.  This is an old patch of Heather's that I reviewed.  I have already verified that this solves the bug and will land this patch.

Comment 27

[cmtalbert]

Landed:
* master: http://github.com/mozautomation/mozmill/commit/5fed21f2bf5dfccda675ed90442a3811511de366
* 1.4.2: http://github.com/mozautomation/mozmill/commit/16475539c55aff658f7eed6990067a3c4f661c56

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Clint, this is the wrong bug you have marked as fixed. This bug is about UTF-8 characters in Mozmill tests, which we don't wanted to fix. Instead bug 559814 is the one about transferring stuff back to the python backend via jsbridge.

Let's update the bugs to lower the confusion. See bug 559814 comment 46 for an explanation.

Comment 29

[Merike Sell (:merike)]

I was looking into how to best fix http://mxr.mozilla.org/comm-central/source/calendar/test/mozmill/testUTF8.js which was checked in assuming that this bug will be fixed eventually.

I noticed that bug 377498 has been fixed and it's now possible to load scripts by specifying the encoding. So I tried changing frame.js like:
-    loader.loadSubScript(uri, module);
+    loader.loadSubScript(uri, module, "UTF-8");

Apart from setting prefs containg UTF-8 all typing and asserts work as expected with that change. Could the resolution here be reconsidered?

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #29)
> -    loader.loadSubScript(uri, module);
> +    loader.loadSubScript(uri, module, "UTF-8");

This change will only work on trunk builds (aka mozilla2.0). There is no back-port planned. Would that additional parameter break Mozmill for older branches? But AFIAK it should be silently ignored.

Clint and Heather, what would you say? Looks like a simple approach to get UTF-8 finally working.

Comment 31

[cmtalbert]

I'd love to take it, seems like it's pretty low risk of regression.

Comment 32

[Merike Sell (:merike)]

Attachment: load scripts as UTF-8

Comment 33

[cmtalbert]

Comment on attachment 514877 [details] [diff] [review]
load scripts as UTF-8

I do believe the extra arg is silently ignored on gecko 1.9.2 and earlier so this is fine (tested in the error console on gecko 1.9.2)

Comment 34

[cmtalbert]

An important fix that can be easily backported to hotfix-1.5, let's take this.

Comment 35

[cmtalbert]

Sorry this took so long to land.  Thanks for the patch Merike!

Hotfix-1.5: https://github.com/mozautomation/mozmill/commit/d25aa8608df131894580750b555a6bf3af92a28e

Master (with a test!): https://github.com/mozautomation/mozmill/commit/1e791ad206d6d07fdc9cddaae5b770bb8d73059a

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #35)
> Master (with a test!):

Clint, the next time the test should make use of expect instead of assert. It doesn't hurt in this case but it wouldn't exit the test function immediately.

Merike, would you have time to verify this fix? Thanks.

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Works fine with Mozmill 1.5.3 from the command line. The IDE is still broken but that should be another bug probably.