1207689 - places.remove_all_history() does not set context to chrome

Status: RESOLVED FIXED

(Testing :: Firefox UI Tests, defect)

Description

[Marc Schifer [mschifer]]

Executing places.remove_all_history() while in 'content' context will throw an exception 


def test_history(self):
        with self.marionette.using_context('content'):
            self.marionette.navigate('http://mozilla.org')
            self.places.remove_all_history()

execute_async_script @places.py, line 129
        inline javascript, line 3
        src: "                let hs = Cc["@mozilla.org/browser/nav-history-service;1"]"

Comment 1

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

Reason therefore is that Cc is not available while staying in content context, but only in chrome context. Given that for most of the other API methods we automatically switch to chrome, we should do it here too with using_context.

Comment 2

[utvar]

Attachment: github pull request

Comment 3

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

Comment on attachment 8666411 [details] [review]
github pull request

Oh, wait. I think it's my fault. We actually should not have to switch the scope, but maybe have. I would first try to expand Cc to `Components.classes` only and if that doesn't work we can switch to chrome. But then please with using_context() and not set_context(). The latter doesn't switch back to the original scope after the code has been run.

Comment 4

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

Utvar, while checking some other older bugs for this Bugzilla component I found bug 1141266 which is actually the same underlying problem. Fixing that other bug will implicitly also fix this bug. And I think pushing a full fix for the problem makes more sense as only fixing a single instance. Would you like to fix them all and take bug 1141266 instead?

Comment 5

[utvar]

Whimboo, I'm glad to try working on bug 1141266, in order to fix both bugs. So when it's ready, I should create just one PR for bug 1141266, against branch mozilla-central?

Comment 6

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

That is totally correct. Yes. Thanks for taking the other bug!

Comment 7

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

So bug 1141266 actually didn't fix this specific issue here. Thinking more about it I think that we are doing the right thing here and specifically should not allow to run methods targeted for chrome in content scope.

Chris, do you think it would make sense to setup a decorator which let us bind methods to a specific scope if there is a restriction? That way we could raise way better understandable failure messages in case the wrong scope is being used.

Comment 8

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

Until we know what to do here I will put this bug back into new state.

Comment 9

[Chris Manchester (limited bugmail, email directly)]

I think you've identified the issue here, most of the methods in places.py will require chrome scope.

Between adding using_context to each method or implementing a decorator, both seem reasonable.

Comment 10

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

Hm, so what I forgot is that we actually cannot read the current scope. Because of that I don't see a way to determine if the correct scope has been set for a method. Unless we have a decorator which explicitly tries to access some scope specific elements or methods. Not sure if that is what we want compared to the implicit scope switching. Personally I don't like the latter. What do you think Chris?

Comment 11

[Chris Manchester (limited bugmail, email directly)]

I don't think implicit scope switching will be a hazard for most tests (I see we do it in a number of places already). If it's a concern in certain cases, I think a general try/except with a logged warning about the method being chrome/content only would work.

Comment 12

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

Sorry for being late in the reply. So lets take the way in implicitly switching the scope in those helper methods which really need a defined scope. For that we should not use "self.marionette.set_context()" but "self.marionette.using_context()".

utvar, do you still want to do this change for the broken method in the places module?

Comment 13

[utvar]

Yes, I'll go ahead and fix this.

Comment 14

[utvar]

Attachment: Github pull request

Comment 15

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

Comment on attachment 8682129 [details] [review]
Github pull request

Looks fine to me. Just fix the small nit I was pointing out and we can get your PR merged. Thanks.

Comment 16

[utvar]

Comment on attachment 8682129 [details] [review]
Github pull request

I removed the line as you requested, but one of the Linux checks failed with what appears to be an install error. Okay to proceed to squash these commits anyway?

Comment 17

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

Yeah, looks like the installer has not been correctly downloaded. Looks like to be related to bug 1219934. I have restarted this Travis build but I expect it to become green. I will do the review now and if all is fine we can get it merged.

Comment 18

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

PR got merged to mozilla-central as:
https://github.com/mozilla/firefox-ui-tests/commit/03db9c3f7f53a9d35dbe74da7911745cf0307dea

Thanks Utvar for this fix! If you want another challenge please let me know.

Comment 19

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

Comment on attachment 8682129 [details] [review]
Github pull request

Add missing r+