580438 - [CLI] duplicate -jsbridge 24242 arguments get specified which are interpreted as URLs

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Jeff Hammel]

we supply the port twice on the command line.  Firefox interprets this as a URL to go to.  We should only supply it once

Around this line:

http://github.com/whimboo/mozmill/blob/master/mozmill/mozmill/__init__.py#L216

Comment 1

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

It would be nice to get this into a maintenance release because this is very confusing and could eventually could cause problems in the future because restoring tabs of the former session after a restart could be affected too.

Jeff, what do you think how difficult it is to fix?

Comment 2

[Jeff Hammel]

(In reply to comment #1)
> It would be nice to get this into a maintenance release because this is very
> confusing and could eventually could cause problems in the future because
> restoring tabs of the former session after a restart could be affected too.
> 
> Jeff, what do you think how difficult it is to fix?

I'm not sure what the problem is exactly so i'm not sure how to fix.  A simple solution would be to (possibly in multiple places) see if -jsbridge is in the cmdargs passed to mozrunner before adding.  I'm not sure if this is the correct fix....probably good enough?  I'll look at it more closely when I get to command line options, but I haven't seen this bug and don't know repro steps.

Comment 3

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

It always happens when running the update tests with our testrun_update.py script from the automation repository. After a restart such a page gets opened.

Comment 4

[Jeff Hammel]

So, I think I've found a few likely candidates as far as why this problem occurs:

- http://github.com/mozautomation/mozmill/blob/master/mozrunner/mozrunner/runner.py#L54 : we use mutable function default arguments.  I have been going through killing these, but this one escaped me (for why this was bad, see http://k0s.org/hg/config/file/70544c7406e2/python/mutable_defaults.py)

- this would probably not cause the error on its own if we always called with cmdargs or always didn't;  however, instead of actually using the API, we do bad touches: http://github.com/mozautomation/mozmill/blob/master/jsbridge/jsbridge/__init__.py#L126 ; so its not surprising that this behaviour occurs

Cleanup may be seen at my pluggable branch, for how not to do this

http://github.com/k0s/mozmill/blob/pluggable/

Comment 5

[cmtalbert]

Requires a lot of CLI refactoring, not in scope for 1.5.1

Comment 6

[Jeff Hammel]

This is probably already fixed on the 2.0 branch, at least for proper use of the API.  See also bug 516429 for an additional (proposed) 

I could never reproduce this.  I suspect its some niche use of the (non)API, but without being able to reproduce, I can't really mark it as closed.

If anyone can send repro steps, its probably not a big deal to fix (if, indeed, its still a problem on master).  Also, it is probably impossible to prevent all improper uses of the API nor is it necessarily desirable.

Comment 7

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

(In reply to comment #6)
> If anyone can send repro steps, its probably not a big deal to fix (if, indeed,
> its still a problem on master).  Also, it is probably impossible to prevent all
> improper uses of the API nor is it necessarily desirable.

Executing our software update tests via the testrun_update.py script from the automation repository normally shows this problem all the time.

Comment 8

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

I figured out that this problem is related to our failing test in bug 616152. I will have to check in detail what's going on here. I hope to be able to give more information soon.

Comment 9

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

(In reply to comment #5)
> Requires a lot of CLI refactoring, not in scope for 1.5.1

It doesn't need any refactoring. It's a simple check if -jsbridge is in the command args or not. Only if it cannot be found, we should add it. This will fix the problem for me and also our broken test on bug 616152. Patch upcoming.

Comment 10

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

Attachment: Patch jsbridge

Comment 11

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

Attachment: Patch v1

Now with fixed white-spaces.

Comment 12

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

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

Clint, what do you think? Even if it is a workaround it will fix problems for our test-runs and it's low risk to take for 1.5.2.

Comment 13

[cmtalbert]

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

Nice catch Henrik!  Looks great.  Please land this on 1.5.2. (And if appropriate, on 2.0 as well -- I'm not sure if we need it on 2.0 due to the CLI refactor.)

Comment 14

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

Landed on hotfix-1.5.2:
https://github.com/mozautomation/mozmill/commit/ee3388253c574a691ffc4ac57e6102e7a6812596

Ok, this ist mostly a workaround. After further investigation I was able to find the real issue:

It's a Firefox bug! Believe it or not, but that's it. The -foreground option is causing this misbehavior we are seeing here. Whatever is specified afterward is interpreted as URL. I will file a bug for it.

Jeff, can you please check that we always push -foreground as the last option?

Comment 15

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

For the core issue see bug 625614.

Comment 16

[Jeff Hammel]

This is landed, I guess? Should i nix the r?

Comment 17

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

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

Yes, but I would still like to get feedback to the latest questions, please.

Comment 18

[Jeff Hammel]

I can't read the source code better than anyone else, but a quick grep doesn't turn up any evidence we use -foreground at all, at least on master

Comment 19

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

Ok, so this option is then added by Firefox itself when it gets restarted i.e. for applying an downloaded update or installing an extension. In such a case the above fix should work because we now add -jsbridge only for the first call and not anytime later. Given that no other command line option will be present after -fullscreen. If it will happen again at some point we would have to push this option to the end of the args array.

Jeff, so you know this is fixed in 2.0? So I don't have to take care about? Can we then mark this bug as fixed?

Comment 20

[Jeff Hammel]

If mozmill is invoked in a bizarre way, it may not be fixed in 2.0 (that is, there is no way via the command line it could happen, but the check does not exist in the API).

Comment 21

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

I tried to find the code in Mozmill 2.0 which adds the jsbridge port to the commandline arguments but failed. Jeff, can you point me to that file and line?

Comment 22

[Jeff Hammel]

(In reply to comment #21)
> I tried to find the code in Mozmill 2.0 which adds the jsbridge port to the
> commandline arguments but failed. Jeff, can you point me to that file and line?

https://github.com/mozautomation/mozmill/blob/master/jsbridge/jsbridge/__init__.py#L131

Comment 23

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

Means this code here is adding the -foreground argument as the last argument, or is there any other code I have to know about which adds even more arguments? 

https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/__init__.py#L554

What happens if consumers subclass the mozmill class and overwrite the runner_args method? In that case additionally added args would be positioned behind -foreground, right? And as we are using a dict, we can't sort the args with -foreground as the last element?

Comment 24

[Jeff Hammel]

We aren't using a dict, to my knowledge

Comment 25

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

(In reply to comment #24)
> We aren't using a dict, to my knowledge

See the link you have given me in comment 22, which shows me:

"return dict(cmdargs=cmdargs)"

Comment 26

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

Oh wait. My bad. cmdargs is a member and gets the array assigned. :/

Ok, so I assume we can say fixed for now on all branches. Sadly I can't test it on 2.0 with our automation scripts yet.

Comment 27

[Jeff Hammel]

this will return a dict like:

 {'cmdargs': ['-jsbridge', '24242', ...]}

The command line arguments will still be ordered

Comment 28

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

(In reply to comment #27)
> this will return a dict like:
> 
>  {'cmdargs': ['-jsbridge', '24242', ...]}
> 
> The command line arguments will still be ordered

But an overwritten runner_args method in a subclass will have to call the method of the super-class before it can add its own values. So in mozmill.runner_args we are pushing -foreground as argument. With other flags added we will end-up exactly with the same problem. I think before we can execute the Firefox process we should make sure that this argument is specified as the last one in the array.

Comment 29

[cmtalbert]

This is fixed, let's open a new bug to track the rest of the fixes needed for 2.0 work (particularly where subclassing the CLI is concerned).

Comment 30

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

Verified fixed with test-runs I did on qa-horus. Yay!