935954 - Mozmill should not be allowed to modify the profile_args and runner_args objects from caller

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

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

As what I have seen while working on mozprofile and some testing with out automation scripts, Mozmill messes around with the callers profile_args and runner_args. This is somewhat bad and can cause unpredicted application behavior, beside duplicated entries in lists. In the following an example:

There is some code which makes use of the Mozmill create() method, and which handles a list of add-ons on its own. As of now Mozmill modifies this original list of add-ons:

    def some_method():
        self.addon_list = []

        profile_args = dict(addons=self.addon_list)
        runner_args = dict(binary=self._application)
        mozmill_args = dict(profile_args=profile_args,
                            runner_args=runner_args)
        
        self._mozmill = mozmill.MozMill.create(**mozmill_args)
        self._mozmill.run(tests)

        print self.addon_list

For the final print I would expect to see an empty list, but instead you currently get:

['/mozilla/code/mozmill/mozmill/mozmill/extension', '/mozilla/code/mozmill/jsbridge/jsbridge/extension']

That means Mozmill itself modifies variables from other tools. I don't think that we should do that, and better do a deepcopy of those passed in references to operate on our own object. Really, if an API has to return a modified version of that object, it should be done via a getter or return value but not silently.

Comment 1

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

Attachment: Patch v1

Andrew, please check my initial comment on this bug for the description. This patch is not finalized yet, given that it misses a real test. For now I would like to know if that is the correct route to go. I don't see a better one, without moving too much overhead to the consumers.

Comment 2

[Andrew Halberstadt [:ahal]]

Comment on attachment 828619 [details] [diff] [review]
Patch v1

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

This makes sense to me, and looks like the right approach. Was this actually causing a problem?

Comment 3

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

(In reply to Andrew Halberstadt [:ahal] from comment #2) 
> This makes sense to me, and looks like the right approach. Was this actually
> causing a problem?

Yes, i have noticed that while working on the add-on stuff in mozbase and by using our automation scripts, which in some cases create a second instance of Mozmill to run another set of tests. Here our list of add-ons has been manipulated by Mozmill so the mozmill and jsbridge extension were listed even twice at the end. With that I also found bug 935436 which is fixed now.

Thanks for your feedback! I will create a proper mutt test for it.

Comment 4

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

Attachment: Patch v1.1

Patch with tests for both profile and runner args.

Comment 5

[Andrew Halberstadt [:ahal]]

Comment on attachment 828845 [details] [diff] [review]
Patch v1.1

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

This looks good, though it doesn't have the change from the previous patch. I've already f+'ed it though, so this r+ applies to everything.

Comment 6

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

Attachment: patch v2

That's happening when you don't do a rebase before the format-patch command. Sorry for that. Here the complete patch with the formerly feedbacked code change by taking over r+.

Comment 7

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

Landed as:
https://github.com/mozilla/mozmill/commit/239786f45c59024efa8513f48eef53bdc6cabfe5 (master)
https://github.com/mozilla/mozmill/commit/7648d6623196b7ea12a10fe8b40085dc90bb51a3 (hotfix-2.0)