Andrew McCreight [:mccr8] Reporter
	Description • 7 years ago

I mentioned that this is serialized in bug 1368170 and Andreas replied:

(In reply to Andreas Tolfsen ‹:ato› from comment #1)
> (In reply to Andrew McCreight [:mccr8] from comment #0)
> > Marionette code tries to serialize Services.appInfo, which has
> > a field QueryInterface, which is a function. Because of the inherits field
> > on Function.prototype, this serialization seems to result in infinite
> > recursion. (Presumably because the inherits method is itself a function.)
> 
> This seems like a bug.  Do you have a link to the exact location where we
> try to serialise Services.appInfo directly?  I Know we extract values from
> it, but we should not be trying to serialise Services.appInfo.

I found two places where it seems to:
> I don't exactly know where it was happening, but the script it was running
> was the same as the one here, which is returning Services.appinfo:
> http://searchfox.org/mozilla-central/source/testing/marionette/harness/
> marionette_harness/runner/base.py#669

(In reply to Andrew McCreight [:mccr8] from comment #3)
> It looks like this test also returns it:
> http://searchfox.org/mozilla-central/source/testing/marionette/harness/
> marionette_harness/tests/unit/test_capabilities.py#17

	Andrew McCreight [:mccr8] Reporter
	Comment 1 • 7 years ago

For bug 1186409, I either need for Marionette to stop serializing this object, or to add Cr.NS_ERROR_FILE_INVALID_PATH as another case (in addition to Cr.NS_ERROR_NOT_IMPLEMENTED) to skip a property in evaluate.toJSON. For some reason, with the changes in that bug, we happen to QI the appinfo object to nsICrashReporter before serializing it, and that interface has some nsIFile property on it that throws that error when accessed.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 4 • 7 years ago

(In reply to Andrew McCreight [:mccr8] from comment #1)
> For bug 1186409, I either need for Marionette to stop serializing this
> object, or to add Cr.NS_ERROR_FILE_INVALID_PATH as another case (in addition
> to Cr.NS_ERROR_NOT_IMPLEMENTED) to skip a property in evaluate.toJSON. For
> some reason, with the changes in that bug, we happen to QI the appinfo
> object to nsICrashReporter before serializing it, and that interface has
> some nsIFile property on it that throws that error when accessed.

I wonder if we should disallow objects that implement XPCOM interfaces from being serialised by Marionette at all?  Alternatively we need to have a serialisation technique for XPCOM objects that looks only at properties and which (somehow) prevents infinite recursions from happening.  This seems hard to achieve.

There are obvious flaws with the way Marionette serialises recursive objects in the first place, but I think maybe there are provisions in the WebDriver specification that prevent this:

    https://w3c.github.io/webdriver/webdriver-spec.html#dfn-execute-a-function-body

In any case, I have submitted two patches: the first removes BaseMarionetteTestRunner.appinfo and replaces uses of it with direct calls to individual properties on Services.appinfo, and the second is a suggestion for disallow nsISupports objects from being serialised at all.

I’m open to critique on the second patch as I’m not entirely sure this is the correct approach.  Thoughts welcome.

	Andrew McCreight [:mccr8] Reporter
	Comment 5 • 7 years ago

(In reply to Andreas Tolfsen ‹:ato› from comment #4)
> I wonder if we should disallow objects that implement XPCOM interfaces from
> being serialised by Marionette at all? 

It does seem a little weird that what gets serialized depends on exactly which QIs have been called.

> Alternatively we need to have a
> serialisation technique for XPCOM objects that looks only at properties and
> which (somehow) prevents infinite recursions from happening.  This seems
> hard to achieve.

I'm fixing the infinite recursion in bug 1368170, by deleting what is actually dead code. I don't think it will be a common issue in practice. I'm not an expert on JS, but I would expect that having an object on its own prototype chain is not going to be a common issue.

Thanks for fixing this.

	Andrew McCreight [:mccr8] Reporter
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8872071 [details]
Bug 1368195 - Disallow XPCOM objects from being serialised by Marionette;

https://reviewboard.mozilla.org/r/143574/#review147306

	Andrew McCreight [:mccr8] Reporter
	Comment 7 • 7 years ago

The patches look reasonable to me, but I'm not familiar enough with Marionette to review them.

	Andrew McCreight [:mccr8] Reporter
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8872070 [details]
Bug 1368195 - Remove BaseMarionetteTestRunner.appinfo;

https://reviewboard.mozilla.org/r/143572/#review147308

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8872070 [details]
Bug 1368195 - Remove BaseMarionetteTestRunner.appinfo;

https://reviewboard.mozilla.org/r/143572/#review147482

::: testing/marionette/harness/marionette_harness/runner/base.py
(Diff revision 1)
> -        if self._appinfo:
> -            return self._appinfo
> -
> -        self.marionette.start_session()
> -        with self.marionette.using_context('chrome'):
> -            self._appinfo = self.marionette.execute_script("""

Why not having an implementation like we have for Puppeteer?

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/puppeteer/firefox/firefox_puppeteer/api/appinfo.py

Maybe this would be more helpful on the Marionette class instead, so tests could more easily access application details.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8872071 [details]
Bug 1368195 - Disallow XPCOM objects from being serialised by Marionette;

https://reviewboard.mozilla.org/r/143574/#review147486

This seems to cause lots of regressions at the moment. I would suggest that you move this out to a separate bug.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 11 • 7 years ago
mozreview-review-reply

Comment on attachment 8872070 [details]
Bug 1368195 - Remove BaseMarionetteTestRunner.appinfo;

https://reviewboard.mozilla.org/r/143572/#review147482

> Why not having an implementation like we have for Puppeteer?
> 
> https://dxr.mozilla.org/mozilla-central/source/testing/marionette/puppeteer/firefox/firefox_puppeteer/api/appinfo.py
> 
> Maybe this would be more helpful on the Marionette class instead, so tests could more easily access application details.

Sure, that sounds like a possible future improvement.  Because it is being used so infrequently, I suggest we add one only once there are more use cases.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 12 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #10)
> Comment on attachment 8872071 [details]
> Bug 1368195 - Disallow XPCOM objects from being serialised by Marionette;
> 
> https://reviewboard.mozilla.org/r/143574/#review147486
> 
> This seems to cause lots of regressions at the moment. I would suggest that
> you move this out to a separate bug.

Since the immediate problem has been rectified, I will drop this commit.  Based on mccr8’s comments earlier, I’m not convinced it’s worth the effort quite yet.

	Andrew McCreight [:mccr8] Reporter
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8872070 [details]
Bug 1368195 - Remove BaseMarionetteTestRunner.appinfo;

https://reviewboard.mozilla.org/r/143572/#review147906

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8872070 [details]
Bug 1368195 - Remove BaseMarionetteTestRunner.appinfo;

https://reviewboard.mozilla.org/r/143572/#review151332

::: testing/marionette/harness/marionette_harness/runner/base.py
(Diff revision 3)
> -    def appinfo(self):
> -        if self._appinfo:
> -            return self._appinfo
> -
> -        self.marionette.start_session()
> -        with self.marionette.using_context('chrome'):
> -            self._appinfo = self.marionette.execute_script("""
> -            try {
> -              return Services.appinfo;
> -            } catch (e) {
> -              return null;
> -            }""")
> -        self.marionette.delete_session()

I think you really just want to replace `appinfo` with an `is_e10s` that starts/deletes a session and queries browserTabsRemoteAutostart specifically.

I would combine the two patches in this series into one.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 18 • 7 years ago
mozreview-review

Comment on attachment 8875686 [details]
Bug 1368195 - Make it possible to test e10s state in MnH;

https://reviewboard.mozilla.org/r/147124/#review151328

Thanks for cleaning up the use of appinfo! I'm sorry you've had to touch Marionette runner code. ;)

::: commit-message-21757:8
(Diff revision 1)
> +The previous commit removed BaseMarionetteTestRunner.appinfo which a
> +Marionette harness test relied upon to verify that the harness threw
> +an assertion when the Firefox instance did not match the desired e10s
> +configuration.
> +
> +To circumvent this problem, this patch introducs a _e10s_from_browser

introducs

::: testing/marionette/harness/marionette_harness/runner/base.py:816
(Diff revision 1)
> +        """Query the browser on whether E10s (Electrolysis) is enabled."""
> +        if self._e10s_from_browser is not None:
> +            return self._e10s_from_browser
> +
> +        if self.marionette is None or self.marionette.session is None:
> +            raise Exception("No Marionette session to query e10s state")

self.e10s will only be used by the runner before a session is started, so we really do need to start/delete a session here.

::: testing/marionette/harness/marionette_harness/runner/base.py:822
(Diff revision 1)
> +
> +        with self.marionette.using_context("chrome"):
> +            self._e10s_from_browser = self.marionette.execute_script(
> +                "return Services.appinfo.browserTabsRemoteAutostart")
> +
> +        return self.is_e10s

`return self._e10s_from_browser`

::: testing/marionette/harness/marionette_harness/runner/base.py:851
(Diff revision 1)
>              try:
>                  device_info = self.marionette.instance.runner.device.dm.getInfo()
>              except Exception:
>                  self.logger.warning('Could not get device info', exc_info=True)
>  
>          self.marionette.start_session()

Please move these session calls into `is_e10s`

::: testing/marionette/harness/marionette_harness/runner/base.py:856
(Diff revision 1)
> -        if self.e10s != appinfo_e10s:
> -            message_e10s = ("BaseMarionetteTestRunner configuration (self.e10s) does "
> +        print "self.is_e10s=%s" % self.is_e10s
> +        print "self.e10s=%s" % self.e10s

Let's remove these print statements, or incorporate them into a helpful logger or exception message.

::: testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_runner.py:8
(Diff revision 1)
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  import manifestparser
>  import pytest
>  
> -from mock import Mock, patch, mock_open, sentinel, DEFAULT
> +from mock import MagicMock, Mock, patch, mock_open, sentinel, DEFAULT

MagicMock doesn't seem to be used anywhere.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 19 • 7 years ago
mozreview-review-reply

Comment on attachment 8872070 [details]
Bug 1368195 - Remove BaseMarionetteTestRunner.appinfo;

https://reviewboard.mozilla.org/r/143572/#review151332

> I think you really just want to replace `appinfo` with an `is_e10s` that starts/deletes a session and queries browserTabsRemoteAutostart specifically.
> 
> I would combine the two patches in this series into one.

I have introduced BaseMarionetteTestRunner.is_e10s in the second patch.  I will squash that into this one, so we will only have one commit.  The new function does query browserTabsRemoteAutostart directly, instead of relying on the marshalling of Services.appinfo.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 20 • 7 years ago
mozreview-review-reply

Comment on attachment 8875686 [details]
Bug 1368195 - Make it possible to test e10s state in MnH;

https://reviewboard.mozilla.org/r/147124/#review151328

> introducs

I dropped this in the squashed commit.

> self.e10s will only be used by the runner before a session is started, so we really do need to start/delete a session here.

I think it’s a terrible idea to start and stop the session here.  You would not expect repercussions from calling a property on a class.  The intention of this check is to make it the responsibility of the caller to ensure a session exists before doing the query.

> `return self._e10s_from_browser`

Why?  This calls itself, which will return self._e10s_from_browser because it was populated in the previous line, thereby having predictable control over the cache.

> Please move these session calls into `is_e10s`

I strongly disagree with this.  See my previous comment.

> Let's remove these print statements, or incorporate them into a helpful logger or exception message.

Oops.  These were not meant to make it into the patch.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 22 • 7 years ago

FWIW this patch cannot be merged yet due to the programming errors shown in https://treeherder.mozilla.org/#/jobs?repo=try&revision=885a81573007&selectedJob=105479739.  Unfortunately, due to https://bugzilla.mozilla.org/show_bug.cgi?id=1371319, I cannot run tests on my local system at the present time.  I will however address these as soon as possible.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 23 • 7 years ago
mozreview-review

Comment on attachment 8872070 [details]
Bug 1368195 - Remove BaseMarionetteTestRunner.appinfo;

https://reviewboard.mozilla.org/r/143572/#review151418

::: testing/marionette/harness/marionette_harness/runner/base.py:849
(Diff revision 4)
> -        appinfo_e10s = self.appinfo.get('browserTabsRemoteAutostart', False)
> -        self.logger.info("e10s is {}".format("enabled" if appinfo_e10s else "disabled"))
> -        if self.e10s != appinfo_e10s:
> -            message_e10s = ("BaseMarionetteTestRunner configuration (self.e10s) does "
> -                            "not match browser appinfo")
> +        self.marionette.start_session()
> +
> +        self.marionette.delete_session()
> +
> +        self.logger.info("e10s is {}".format("enabled" if self.is_e10s else "disabled"))
> +        if self.e10s != self.is_e10s:
>              self.cleanup()
> -            raise AssertionError(message_e10s)
> +            raise AssertionError("BaseMarionetteTestRunner configuration (self.e10s) "
> +                "does not match browser appinfo (self.is_e10s)")

(Opening new issue since the previous ones aren't easily discoverable in MozReview after squashing commits.)

I agree with your argument about making the session management a responsibility of the caller of `is_e10s`. 

In any case, here `delete_session` is being called too soon.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 24 • 7 years ago

(In reply to Andreas Tolfsen ‹:ato› from comment #20) 
> > `return self._e10s_from_browser`
> 
> Why?  This calls itself, which will return self._e10s_from_browser because
> it was populated in the previous line, thereby having predictable control
> over the cache.

Yes, it's correct but it's an extra method call with no added benefit. Why not just return the variable that was populated in the previous line? How would that decrease predictability? 

Happy to drop the issue, but curious where we're not understanding each other.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 25 • 7 years ago
mozreview-review-reply

Comment on attachment 8872070 [details]
Bug 1368195 - Remove BaseMarionetteTestRunner.appinfo;

https://reviewboard.mozilla.org/r/143572/#review151418

> (Opening new issue since the previous ones aren't easily discoverable in MozReview after squashing commits.)
> 
> I agree with your argument about making the session management a responsibility of the caller of `is_e10s`. 
> 
> In any case, here `delete_session` is being called too soon.

Sorry, I didn’t mean for mozreview to silently drop the issue.  Thanks for raising one to track the issue again.

I will look into the case of calling delete_session too soon once the Linux build doesn’t crash on startup anymore.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 26 • 7 years ago

(In reply to Maja Frydrychowicz (:maja_zf) from comment #23)
> In any case, here `delete_session` is being called too soon.

This should now be fixed.

> `return self._e10s_from_browser`

I guess I don’t care enough about this, and I’ve changed it to your suggestion.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 28 • 7 years ago
mozreview-review

Comment on attachment 8872070 [details]
Bug 1368195 - Remove BaseMarionetteTestRunner.appinfo;

https://reviewboard.mozilla.org/r/143572/#review152050

::: testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_runner.py:10
(Diff revision 5)
>  import manifestparser
>  import pytest
>  
>  from mock import Mock, patch, mock_open, sentinel, DEFAULT
>  
> +from marionette_driver.marionette import Marionette

unused import?

	Pulsebot
	Comment 30 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c932f9b1d502
Remove BaseMarionetteTestRunner.appinfo; r=maja_zf,whimboo

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 31 • 7 years ago

Backed out for flake8 linting failure at base.py:855:17 | continuation line under-indented for visual indent (E128):

https://hg.mozilla.org/integration/autoland/rev/8bc95cfe213be47722cc9aca936df817eb1737aa

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c932f9b1d50205757d812d860dd30b3678f912f3&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=106358782&repo=autoland
> TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/testing/marionette/harness/marionette_harness/runner/base.py:855:17 | continuation line under-indented for visual indent (E128)

	Pulsebot
	Comment 33 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a1732661b9d
Remove BaseMarionetteTestRunner.appinfo; r=maja_zf,whimboo

	Ryan VanderMeulen [:RyanVM]
	Comment 34 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/7a1732661b9d