Mozmill adds collected addons as string but not as object to the report

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

({regression})

Details

(Whiteboard: [mozmill-2.0+])

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
Compared to our code in the mozmill-automation repository Mozmill adds the collected add-on details as a big string instead of a JSON object. That's causing problems in parsing the report and getting the data out for our dashboard.

I think that's something which is easy enough to fix and important to have for 2.0.
(Assignee)

Updated

6 years ago
Whiteboard: [mozmill-2.0?]
(Assignee)

Comment 1

6 years ago
The initial implementation has been done on bug 636773.
Blocks: 636773
(Assignee)

Comment 2

6 years ago
Created attachment 664142 [details]
Patch

Pointer to Github pull-request
(Assignee)

Comment 3

6 years ago
Comment on attachment 664142 [details]
Patch

This patch ensures that we do not stringify the addon details twice but now converts even the whole app_info object into a unicode string. That should make us much safer. I believe we should do that for any kind of data we transfer. That's why I put the code into the utils.js module for re-usage. I will file a follow-up bug for it.

As a sidenote we now add a '?' if a character could not be converted. We do no longer use '' because that could cause faulty output where you can't see that there was such a broken character.
Attachment #664142 - Attachment description: Pointer to Github pull request: https://github.com/mozilla/mozmill/pull/103 → Patch
Attachment #664142 - Flags: review?(jhammel)

Comment 4

6 years ago
Comment on attachment 664142 [details]
Patch

lgtm
Attachment #664142 - Flags: review?(jhammel) → review+
(Assignee)

Comment 5

6 years ago
https://github.com/mozilla/mozmill/commit/cc8dc7b7f4f53cae5bf470250bb02c5d450c8ca0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Depends on: 794885
(Assignee)

Comment 6

6 years ago
I had to backout the patch because it's causing test failures on Windows and Linux. See bug 794885.

https://github.com/mozilla/mozmill/commit/750a288ec1d7adf399dce1f8a4b78db78e25ddc8
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 7

6 years ago
So the problem we see here on Windows and Linux is:

Traceback (most recent call last):
  File "z:\data\code\mozmill\mozmill\mozmill\__init__.py", line 751, in run
    mozmill.run(tests, self.options.restart)
  File "z:\data\code\mozmill\mozmill\mozmill\__init__.py", line 393, in run
    frame = self.run_test_file(frame or self.start_runner(),
  File "z:\data\code\mozmill\mozmill\mozmill\__init__.py", line 317, in start_runner
    self.results.appinfo = self.get_appinfo(self.bridge)
  File "z:\data\code\mozmill\mozmill\mozmill\__init__.py", line 427, in get_appinfo
    app_info = json.loads(mozmill.getApplicationDetails())
  File "c:\mozmill-2-env\python\Lib\json\__init__.py", line 326, in loads
    return _default_decoder.decode(s)
  File "c:\mozmill-2-env\python\Lib\json\decoder.py", line 366, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
TypeError: expected string or buffer
Status: REOPENED → ASSIGNED
(Assignee)

Comment 8

6 years ago
This is totally strange. The problem we have here is located at:

converter.ConvertToUnicode(JSON.stringify(details, replacer));

And it only happens when add-ons contain special characters like '®'. The code above works totally fine when it gets called from within the extension itself means when we e.g. collect the addons. But it miserably fails when we call:

app_info = json.loads(mozmill.getApplicationDetails())

from the Python side. The content will be the same so something in the calling chain via JSBridge is causing us problems here. 

When I execute the same code in a test which gets the same JSON data the issue is not there. So it's really only happening in the framework. I would assume it is related to the way how we call the JS function.
(Assignee)

Comment 9

6 years ago
When I enable debugging for JSBridge I can see that the following code in mozmill.js gets garbled:

mozmill.js:    var json = {"name":"®"};
debug output:  var json = {"name":"®"};

The locale in my terminal window is set to UTF-8.
(Assignee)

Comment 10

6 years ago
I think that something fundamental is totally broken with JSBrige here. Also I don't think it's worth fixing. Better we switch to Marionette in the future. So I will revert the changes and modify get_appinfo() to extra decode the addons string to an object. I hope that at least this will work.
(Assignee)

Comment 11

6 years ago
Created attachment 665830 [details]
Patch v2

Pointer to Github pull-request
(Assignee)

Comment 12

6 years ago
Comment on attachment 665830 [details]
Patch v2

This patch is less invasive and works across platforms. The UTF-8 issue we should follow-up on bug 761603.
Attachment #665830 - Attachment description: Pointer to Github pull request: https://github.com/mozilla/mozmill/pull/104 → Patch v2
Attachment #665830 - Flags: review?(jhammel)
(Assignee)

Comment 13

6 years ago
Clint, I hope you can remember why you have changed the code which sends the addons to Python:

https://github.com/mozilla/mozmill/commit/e173ad60b0c7d7ab664c5a7c29fb573a4f735e89#L0L429

Would sending the object directly also have caused a startup slowness? I wouldn't imagine so because the the object is not that large. Probably I should measure the timing. If possible I would go back to transfer mozmill.addons directly.
(Assignee)

Comment 14

6 years ago
I have tested both ways and the time difference between the former and latter code is only about 200ms. So I would keep the functionality as it is right now and would put any more thoughts on bug 795282 in the future.

Comment 15

6 years ago
(In reply to Henrik Skupin (:whimboo) from comment #13)
> Clint, I hope you can remember why you have changed the code which sends the
> addons to Python:
> 
> https://github.com/mozilla/mozmill/commit/
> e173ad60b0c7d7ab664c5a7c29fb573a4f735e89#L0L429
> 
> Would sending the object directly also have caused a startup slowness? I
> wouldn't imagine so because the the object is not that large. Probably I
> should measure the timing. If possible I would go back to transfer
> mozmill.addons directly.

IIRC, Clint measured a rather large slowdown because of the way jsbridge works (e.g. crawling all of object space and transfering one attribute at a time).  I hope he can elaborate

Comment 16

6 years ago
Comment on attachment 665830 [details]
Patch v2

Pretty hacky, but probably the best way forward.
Attachment #665830 - Flags: review?(jhammel) → review+

Comment 17

6 years ago
(In reply to Jeff Hammel [:jhammel] from comment #15)
> (In reply to Henrik Skupin (:whimboo) from comment #13)
> > Clint, I hope you can remember why you have changed the code which sends the
> > addons to Python:
> > 
> > https://github.com/mozilla/mozmill/commit/
> > e173ad60b0c7d7ab664c5a7c29fb573a4f735e89#L0L429
> > 
> > Would sending the object directly also have caused a startup slowness? I
> > wouldn't imagine so because the the object is not that large. Probably I
> > should measure the timing. If possible I would go back to transfer
> > mozmill.addons directly.
> 
> IIRC, Clint measured a rather large slowdown because of the way jsbridge
> works (e.g. crawling all of object space and transfering one attribute at a
> time).  I hope he can elaborate
The sending wasn't a problem. It was the repeated accessing. Each time we accessed the object that jsbridge gave us back to pull a piece of data out we did a full roundtrip through jsbridge and we had to re-construct the in-memory object every single time (because javascript properties can change at any time). And this was what was incredibly expensive.  As long as you only do one trip over the bridge, and use the returned JSON to pull data out in python (rather than accessing the object returned from jsbridge directly) then you'll be fine and won't reintroduce the performance issue.
(Assignee)

Comment 18

6 years ago
Yes, I have tested it and any remaining improvement we can do on bug 795282. For now I don't see a way how to make it more effective or nicer. I really hope that we will not have such a problem with Marionette later in the future.

Pushed to master:
https://github.com/mozilla/mozmill/commit/1558c2b7b8d982b2b9abc753545d8f4eaf501318
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Keywords: regression
Resolution: --- → FIXED
Whiteboard: [mozmill-2.0?] → [mozmill-2.0+]

Comment 19

6 years ago
Marionette is transactional, in the traditional sense, so this problem should not occur.  JSBridge, on the other hand, "silently" performs transacations implicitly.  It is cute, but less good for things such as fetching an entire state at once
(Assignee)

Updated

5 years ago
Blocks: 877100
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.