635237 - Drop support for assertJS and waitForEval in Mozmill 2.0

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

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

Both assertJS and waitForEval are using the eval method to evaluate the given expression. This is faulty, can cause security holes, and should even not be ready for e10s. We should always use the callback way to make it asynchronous.

The major jump to Mozmill 2.0 is the best time to get this fixed.

Comment 1

[cmtalbert]

How does this work with/leverage the new assertion modules?

Comment 2

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

Would be a good replacement! The assertions module has been landed today in our refactoring repo and will be tested in the next couple of weeks.

Comment 3

[Andrew Halberstadt [:ahal]]

So is this bug basically just "Remove assertJS and waitForEval" from the controller?  I can roll this up into the controller refactor if so.

Comment 4

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

From my standpoint yes. But I would like to get feedback from Clint.

Comment 5

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.0 - Remove assertJS and waitForEval

Clint, is this what was meant by drop support?

Comment 6

[Andrew Halberstadt [:ahal]]

Actually, should I remove waitForEval from utils.js as well?

Comment 7

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

(In reply to comment #6)
> Actually, should I remove waitForEval from utils.js as well?

That's what this bug is about: assertJS and waitForEval. So I would say yes.

Comment 8

[Andrew Halberstadt [:ahal]]

Hmm, so I did a grep through the Thunderbird test automation and they seem to use "waitForEval" fairly frequently: http://pastebin.com/3bJJMb7F

Comment 9

[Andrew Halberstadt [:ahal]]

On second look... I don't know why they are using waitForEval.. they could just use waitFor in (I think) all those cases.

Comment 10

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

Mark, do you have any objections in getting the waitForEval crap removed? It's insecure and with waitFor we have a much better way to get that done.

Comment 11

[Mark Banner (:standard8)]

I'll let Andrew comment on if we need those or not.

Comment 12

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

Just for an example how waitFor and assert works, you can check:
http://hg.mozilla.org/qa/mozmill-tests/file/default/tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js#l123

For Mozmill 2.0 we will have a complete new assertion library but waitFor will persist. See bug 637880.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

I hate eval too; waitFor() does not appear to exist in MozMill 1.4.2 (and definitely did not exist when we started using mozmill), so we are not using it.  It seems completely reasonable to remove it for 2.0 and we can change our existing usages.

Do the new waitFor or other spin options support an optimized, non-polling form of loop spinning (backed by a single timer performing a timeout)?  Something like a promise?  A lot of the notifications we wait for in Thunderbird are actually event driven, so we end up setting some intermediary variable which the waitForEval then checks on a polling basis, which is less than optimal.

Comment 14

[Andrew Halberstadt [:ahal]]

I'll remove waitForEval completely (from utils.js) pending ok from Clint.

Comment 15

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

(In reply to comment #13)
> Do the new waitFor or other spin options support an optimized, non-polling form
> of loop spinning (backed by a single timer performing a timeout)?  Something
> like a promise?  A lot of the notifications we wait for in Thunderbird are
> actually event driven, so we end up setting some intermediary variable which
> the waitForEval then checks on a polling basis, which is less than optimal.

Andrew, you should check the following class which is available pre 1.4.2:
https://github.com/mozautomation/mozmill/blob/hotfix-1.5.2/mozmill/mozmill/extension/resource/modules/controller.js#L63

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #15)
> Andrew, you should check the following class which is available pre 1.4.2:
> https://github.com/mozautomation/mozmill/blob/hotfix-1.5.2/mozmill/mozmill/extension/resource/modules/controller.js#L63

That's not quite what I'm looking for; Thunderbird rarely generates DOM-style events for notifications, and it is still backed by the timer-based polling mechanism for loop-spinning (even on the master branch).

What I want would be more like:
let spinner = controller.gimmeSpinnerWithTimeout(ONE_SECOND);
thunderbird.goDoSomething(spinner.thingToCallWhenAllDone);
spinner.spinSpinSpinSpin();

Where the spin function would spin the event loop until either thingToCallWhenAllDone is invoked or the timeout expires.  The timer would be canceled on success.

Comment 17

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

(In reply to comment #16)
> That's not quite what I'm looking for; Thunderbird rarely generates DOM-style
> events for notifications, and it is still backed by the timer-based polling
> mechanism for loop-spinning (even on the master branch).

Can you please file that as a new bug?

Comment 18

[Andrew Halberstadt [:ahal]]

Comment on attachment 522450 [details] [diff] [review]
Patch 1.0 - Remove assertJS and waitForEval

I talked to Clint and he said to just go ahead and take out the methods.

master: https://github.com/mozautomation/mozmill/commit/d02d4322e18ec96697c58d2904c7532df7fae995