1452490 - Remove marionetteScriptFinished

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P2)

Description

[Andreas Tolfsen ❲:ato❳]

marionetteScriptFinished is a global exposed to all scripts injected
using WebDriver:ExecuteAsyncScript.  It is a non-conforming global
that we do not want to expose.

Instead, scripts should be relying on the last argument from the
arguments local scoped variable to return.

See
https://searchfox.org/mozilla-central/search?q=marionetteScriptFinished&path=
for a list of usages in central.

Comment 1

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

(In reply to Andreas Tolfsen ‹:ato› from comment #0)
> Instead, scripts should be relying on the last argument from the
> arguments local scoped variable to return.

Can you please explain this a bit more? It's not that clear to me. Thanks.

Comment 2

[Andreas Tolfsen ❲:ato❳]

Scripts are wrapped in functions that are passed a set of arguments.
In addition to any user-defined arguments, async scripts  always
gets an additional last argument that is a callback or resolve
function.

For example:

> self.marionette.execute_script("""
>     let [foo, resolve] = arguments;
>     setTimeout(() => resolve(foo), 1000);
>     """, script_args("foo",))

Comment 3

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

Here an updated link for the currently in use marionetteScriptFinished method:

https://dxr.mozilla.org/mozilla-central/search?q=marionetteScriptFinished&=mozilla-central

I think this is a good bug for mentoring, so I will take it.

Comment 4

[Venkatesh (:vpit3833)]

I started by removing sandBoxName and MARIONETTE_SCRIPT_FINISHED from evaluate.js, and ./mach marionette test returns error for marionetteScriptFinished not defined.  How to proceed from here?

Comment 5

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

You basically want to start with all the tests which are using `marionetteScriptFinished`, and turn them to the new format. As last step you can then remove the implementation in Marionette itself.

Comment 6

[Venkatesh (:vpit3833)]

Can you give an example please?

For instance, at https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py#21, what will the new format be looking like?

Comment 7

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

Sure. So here the original:

> self.assertEqual(1, self.marionette.execute_async_script("marionetteScriptFinished(1);"))

Which means that this script tests for the return value of the async script command. Given that we want to get rid of `marionetteScriptFinished` you will have to replace it with `resolve(1)` as I mentioned in my last comment. Then from Andreas' example in comment 2 you can see how to get the reference to resolve. Which means what it will look like this:

> self.assertEqual(1, self.marionette.execute_async_script("let [resolve] = arguments; resolve(1);"))

Comment 8

[Venkatesh (:vpit3833)]

Attachment: Bug 1452490 - Remove marionetteScriptFinished.

Review commit: https://reviewboard.mozilla.org/r/250358/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250358/

Comment 9

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8984507 [details]
Bug 1452490 - Remove marionetteScriptFinished.

https://reviewboard.mozilla.org/r/250358/#review256598

::: testing/awsy/awsy/awsy_test_case.py:151
(Diff revision 1)
>          gc_script = """
>              Cu.import("resource://gre/modules/Services.jsm");
>              Services.obs.notifyObservers(null, "child-mmu-request", null);
>  
>              let memMgrSvc = Cc["@mozilla.org/memory-reporter-manager;1"].getService(Ci.nsIMemoryReporterManager);
> -            memMgrSvc.minimizeMemoryUsage(() => marionetteScriptFinished("gc done!"));
> +            memMgrSvc.minimizeMemoryUsage(() => {let [resolve] = arguments; resolve("gc done!");});

I would assign resolve at the beginning of the function.

::: testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py:22
(Diff revision 1)
>  
>      def test_execute_async_simple(self):
>          self.assertEqual(1, self.marionette.execute_async_script("arguments[arguments.length-1](1);"))
>  
>      def test_execute_async_ours(self):
> -        self.assertEqual(1, self.marionette.execute_async_script("marionetteScriptFinished(1);"))
> +        self.assertEqual(1, self.marionette.execute_async_script("let [resolve] = arguments; resolve(1);"))

In all these cases you could just call arguments[0](1).

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_close_content.py:126
(Diff revision 1)
> -                marionetteScriptFinished(true);
> +                let [resolve] = arguments;
> +                resolve(true);

Assign resolve at beginning of function.

Comment 10

[Venkatesh (:vpit3833)]

Comment on attachment 8984507 [details]
Bug 1452490 - Remove marionetteScriptFinished.

https://reviewboard.mozilla.org/r/250358/#review256598

> In all these cases you could just call arguments[0](1).

Done, using arguments[0](xyz).

Before submitting next review,

How to proceed here: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py#103 ?  arguments[2] is None, and 'let [resolve] = arguments; resolve();' returns TypeError on resolve as it is not a function.  This is the last place still using marionetteScriptFinished.  When this is done, evaluate.js can be edited as well.

Comment 11

[Venkatesh (:vpit3833)]

Comment on attachment 8984507 [details]
Bug 1452490 - Remove marionetteScriptFinished.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250358/diff/1-2/

Comment 12

[Venkatesh (:vpit3833)]

Comment on attachment 8984507 [details]
Bug 1452490 - Remove marionetteScriptFinished.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250358/diff/1-2/

Comment 13

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8984507 [details]
Bug 1452490 - Remove marionetteScriptFinished.

https://reviewboard.mozilla.org/r/250358/#review257034

::: testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py:28
(Diff revision 2)
>  
>      def test_execute_async_timeout(self):
>          self.assertRaises(ScriptTimeoutException, self.marionette.execute_async_script, "var x = 1;")
>  
>      def test_execute_async_unique_timeout(self):
> -        self.assertEqual(2, self.marionette.execute_async_script("setTimeout(function() {marionetteScriptFinished(2);}, 2000);", script_timeout=5000))
> +        self.assertEqual(2, self.marionette.execute_async_script("setTimeout(arguments[0](2), 2000);", script_timeout=5000))

Here you are calling arguments[0] literally, whereas the code before
passed an anonymous function calling the callback.  Because
arguments[0](2) returns 2 immediately you need to still keep the
anonymous function here.

The correct code would be:

> setTimeout(() => arguments[0](2), 2000);

::: testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py:102
(Diff revision 2)
> +                                             # "let [resolve] = arguments;"
> +                                             # "resolve();",

Debug?

::: testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py:104
(Diff revision 2)
>  
>      def test_sandbox_refresh_arguments(self):
>          self.marionette.execute_async_script("this.foobar = [arguments[0], arguments[1]];"
> +                                             # "let [resolve] = arguments;"
> +                                             # "resolve();",
>                                               "marionetteScriptFinished();",

Remaining use of marionetteScriptFinished.

::: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py:117
(Diff revision 2)
> -                       .then(() => marionetteScriptFinished(true))
> -                       .catch(() => marionetteScriptFinished(false));
> +                       .then(() => arguments[0](true))
> +                       .catch(() => arguments[0](false));

Here too I would assign resolve first so that you don’t have to
index arguments twice.

Comment 14

[Venkatesh (:vpit3833)]

Comment on attachment 8984507 [details]
Bug 1452490 - Remove marionetteScriptFinished.

https://reviewboard.mozilla.org/r/250358/#review257034

> Remaining use of marionetteScriptFinished.

Yes, this is where I don't know how to go ahead.  replacing this with

> let [resolve] = arguments;
> resolve();

returns type error on resolve as it is not a function but an array.  So I commented the two lines to remind me to not miss the spot.  Do you recommend any pointers to follow to find a solution?

Comment 15

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8984507 [details]
Bug 1452490 - Remove marionetteScriptFinished.

https://reviewboard.mozilla.org/r/250358/#review257034

> Yes, this is where I don't know how to go ahead.  replacing this with
> 
> > let [resolve] = arguments;
> > resolve();
> 
> returns type error on resolve as it is not a function but an array.  So I commented the two lines to remind me to not miss the spot.  Do you recommend any pointers to follow to find a solution?

Ah sorry, I missed your question earlier.  Thanks for bringing it
to my attention again.

The callback is always the last argument, so what you will want to
do here is something like this:

> let resolve = arguments[arguments.length - 1];

Or:

> let [x, y, resolve] = arguments;

Or:

> let resolve = arguments[2];

Comment 16

[Venkatesh (:vpit3833)]

Comment on attachment 8984507 [details]
Bug 1452490 - Remove marionetteScriptFinished.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250358/diff/2-3/

Comment 17

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

Venkatesh, is the last update of the patch ready to review? I asked because you didn't set the review flag.

Comment 18

[Venkatesh (:vpit3833)]

Ah, I missed it Henrik.  Yes, this is the recent update of the patch for review. The comment block refers to marionetteScrioptFinished to explain things.  For now I have replaced it with ?s

Comment 19

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

Ok, so to practice can you please set the review flag to me in mozreview?

Comment 20

[Venkatesh (:vpit3833)]

In this try server output at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0bca58f77baa0a205559dfadab7cbe27f1fc771, how to find out where the fails are originating from?

Says there are 20 fails.

Comment 21

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

(In reply to Venkatesh from comment #20)
> In this try server output at
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e0bca58f77baa0a205559dfadab7cbe27f1fc771, how to find
> out where the fails are originating from?

Click on each of those failing jobs and you will notice a new panel is opening at the bottom. Failing tests are marked with `TEST-UNEXPECTED-ERROR`. Check the path as printed directly next to it, and run this test locally. Those should also be broken, and would need an update.

I will review the patch once the failures are gone.

Comment 22

[Andreas Tolfsen ❲:ato❳]

I think whimboo mostly answered this, but the failures all seem to
be in the Firefox UI tests.  When you click the orange en-US icons
next to the test type, you will find a list of test errors originating
from files such as testing/firefox-ui/tests/puppeteer/test_places.py.

Ping me on IRC (ato) if you need more help.

Comment 23

[Venkatesh (:vpit3833)]

./mach test testing/firefox-ui/tests/puppeteer/test_places.py
 0:00.02 ERROR Failure during harness execution
Traceback (most recent call last):

  File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/runtests.py", line 94, in cli
    failed = harness_instance.run()

  File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/runtests.py", line 72, in run
    runner = self._runner_class(**self.args)

  File "/home/venkatesh/src/mozilla-central/testing/firefox-ui/harness/firefox_ui_harness/runners/base.py", line 18, in __init__
    super(FirefoxUITestRunner, self).__init__(**kwargs)

  File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/runner/base.py", line 532, in __init__
    self.test_kwargs = deepcopy(kwargs)

  File "/usr/lib/python2.7/copy.py", line 163, in deepcopy
    y = copier(x, memo)

  File "/usr/lib/python2.7/copy.py", line 257, in _deepcopy_dict
    y[deepcopy(key, memo)] = deepcopy(value, memo)

  File "/usr/lib/python2.7/copy.py", line 190, in deepcopy
    y = _reconstruct(x, rv, 1, memo)

  File "/usr/lib/python2.7/copy.py", line 334, in _reconstruct
    state = deepcopy(state, memo)

  File "/usr/lib/python2.7/copy.py", line 163, in deepcopy
    y = copier(x, memo)

  File "/usr/lib/python2.7/copy.py", line 257, in _deepcopy_dict
    y[deepcopy(key, memo)] = deepcopy(value, memo)

  File "/usr/lib/python2.7/copy.py", line 190, in deepcopy
    y = _reconstruct(x, rv, 1, memo)

  File "/usr/lib/python2.7/copy.py", line 334, in _reconstruct
    state = deepcopy(state, memo)

  File "/usr/lib/python2.7/copy.py", line 163, in deepcopy
    y = copier(x, memo)

  File "/usr/lib/python2.7/copy.py", line 257, in _deepcopy_dict
    y[deepcopy(key, memo)] = deepcopy(value, memo)

  File "/usr/lib/python2.7/copy.py", line 163, in deepcopy
    y = copier(x, memo)

  File "/usr/lib/python2.7/copy.py", line 230, in _deepcopy_list
    y.append(deepcopy(a, memo))

  File "/usr/lib/python2.7/copy.py", line 190, in deepcopy
    y = _reconstruct(x, rv, 1, memo)

  File "/usr/lib/python2.7/copy.py", line 334, in _reconstruct
    state = deepcopy(state, memo)

  File "/usr/lib/python2.7/copy.py", line 163, in deepcopy
    y = copier(x, memo)

  File "/usr/lib/python2.7/copy.py", line 257, in _deepcopy_dict
    y[deepcopy(key, memo)] = deepcopy(value, memo)

  File "/usr/lib/python2.7/copy.py", line 163, in deepcopy
    y = copier(x, memo)

  File "/usr/lib/python2.7/copy.py", line 230, in _deepcopy_list
    y.append(deepcopy(a, memo))

  File "/usr/lib/python2.7/copy.py", line 190, in deepcopy
    y = _reconstruct(x, rv, 1, memo)

  File "/usr/lib/python2.7/copy.py", line 334, in _reconstruct
    state = deepcopy(state, memo)

  File "/usr/lib/python2.7/copy.py", line 163, in deepcopy
    y = copier(x, memo)

  File "/usr/lib/python2.7/copy.py", line 257, in _deepcopy_dict
    y[deepcopy(key, memo)] = deepcopy(value, memo)

  File "/usr/lib/python2.7/copy.py", line 190, in deepcopy
    y = _reconstruct(x, rv, 1, memo)

  File "/usr/lib/python2.7/copy.py", line 334, in _reconstruct
    state = deepcopy(state, memo)

  File "/usr/lib/python2.7/copy.py", line 163, in deepcopy
    y = copier(x, memo)

  File "/usr/lib/python2.7/copy.py", line 257, in _deepcopy_dict
    y[deepcopy(key, memo)] = deepcopy(value, memo)

  File "/usr/lib/python2.7/copy.py", line 182, in deepcopy
    rv = reductor(2)

TypeError: __getnewargs__ should return a tuple, not 'unicode'

What is going wrong here?  I am trying to run the failed tests locally and getting this.

Comment 24

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

(In reply to Venkatesh from comment #23)
> "/home/venkatesh/src/mozilla-central/testing/marionette/harness/
> marionette_harness/runner/base.py", line 532, in __init__
>     self.test_kwargs = deepcopy(kwargs)

You want to check which entries `kwargs` contains, and figure out which of those causing `deepcopy` to fail.

Comment 25

[Venkatesh (:vpit3833)]

(In reply to Henrik Skupin (:whimboo) from comment #24)
> 
> You want to check which entries `kwargs` contains, and figure out which of
> those causing `deepcopy` to fail.

I get

kwargs = {'package_name': None, 'log_mach_level': None, 'test_objects': [{'name': 'test_places.py', 'tags': 'local', 'here': u'/home/venkatesh/src/mozilla-central/testing/firefox-ui/tests/puppeteer', 'manifest': u'/home/venkatesh/src/mozilla-central/testing/firefox-ui/tests/puppeteer/manifest.ini', u'file_relpath': u'testing/firefox-ui/tests/puppeteer/test_places.py', 'path': u'/home/venkatesh/src/mozilla-central/testing/firefox-ui/tests/puppeteer/test_places.py', u'flavor': u'firefox-ui-functional', u'dir_relpath': u'testing/firefox-ui/tests/puppeteer', 'relpath': u'testing/firefox-ui/tests/puppeteer/test_places.py'}], 'emulator_bin': None, 'avd': None, 'prefs_files': None, 'jsdebugger': False, 'log_errorsummary': None, 'log_xunit': None, 'log_mach': None, 'log_tbpl_level': None, 'log': <mozlog.structuredlog.StructuredLogger object at 0x7f26e7cd0ed0>, 'log_tbpl': None, 'log_unittest': None, 'avd_home': None, 'pydebugger': None, 'log_tbpl_compact': None, 'log_tbpl_buffer': None, 'log_raw': None, 'logger_name': 'Marionette-based Tests', 'log_mach_buffer': None, 'log_mach_verbose': None, 'log_raw_level': None, 'adb_path': None, 'prefs_args': None, 'log_html': None, 'device_serial': None}

How to work out which of these is the trouble?

Comment 26

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

I don't see anything obvious here. Which version of Python are you using? Maybe also pull and update your local code for latest additions on central. Maybe there was a broken changeset.

Comment 27

[Andreas Tolfsen ❲:ato❳]

I’m seeing the same thing as venkatesh when running "MOZ_HEADLESS=1
./mach test testing/firefox-ui/tests/puppeteer/test_places.py" on
central (without this patch applied).  I’ve filed
https://bugzilla.mozilla.org/show_bug.cgi?id=1470397 about this.

To run this test it is possible to use "./mach firefox-ui-test
TEST".

Comment 28

[Andreas Tolfsen ❲:ato❳]

(In reply to Andreas Tolfsen 「:ato」 from comment #27)

> To run this test it is possible to use "./mach firefox-ui-test
> TEST".

I mean "./mach firefox-ui-functional TEST".

Comment 29

[Andreas Tolfsen ❲:ato❳]

It appears that the problem with the tests is that resolve is undefined:

> JavaScript error: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py, line 43: TypeError: resolve is not a function

This fixes the problem:

> diff --git a/testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py b/testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py
> index cc1a8e5e43f0..7072bca13872 100644
> --- a/testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py
> +++ b/testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py
> @@ -32,8 +32,8 @@ class Places(BaseLib):
>          return self.marionette.execute_async_script("""
>            Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
>  
> -          let [resolve] = arguments;
> -          PlacesUtils.bookmarks.fetch({url: resolve}).then(bm => {
> +          let [url, resolve] = arguments;
> +          PlacesUtils.bookmarks.fetch({url}).then(bm => {
>              resolve(bm != null);
>            });
>          """, script_args=[url])
> @@ -49,13 +49,13 @@ class Places(BaseLib):
>            Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
>  
>            let folderGuids = [];
> -          let [resolve] = arguments;
> +          let [url, resolve] = arguments;
>  
>            function onResult(bm) {
>              folderGuids.push(bm.parentGuid);
>            }
>  
> -          PlacesUtils.bookmarks.fetch({url: resolve}, onResult).then(() => {
> +          PlacesUtils.bookmarks.fetch({url}, onResult).then(() => {
>              resolve(folderGuids);
>            });
>          """, script_args=[url])

Comment 30

[Venkatesh (:vpit3833)]

(In reply to Andreas Tolfsen 「:ato」 from comment #29)
> It appears that the problem with the tests is that resolve is undefined:
> 
> > JavaScript error: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py, line 43: TypeError: resolve is not a function
> 

How to arrive at this error message?  Because test_utils.py, test_software_update.py, and test_places.py all return the same Python error message.  They all might share same/similar fix as well.

And, where is this patch pasted here to be applied? on the bookmark for this bug?  mozilla-central?

Comment 31

[Andreas Tolfsen ❲:ato❳]

(In reply to Venkatesh from comment #30)

> (In reply to Andreas Tolfsen 「:ato」 from comment #29)
> 
> > It appears that the problem with the tests is that resolve is undefined:
> > 
> > > JavaScript error: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py, line 43: TypeError: resolve is not a function
> 
> How to arrive at this error message?  Because test_utils.py,
> test_software_update.py, and test_places.py all return the same Python error
> message.  They all might share same/similar fix as well.

Try looking at what Firefox writes to stdout with the "--gecko-log -" argument.

> And, where is this patch pasted here to be applied? on the bookmark for this
> bug?  mozilla-central?

It was applied on top your patch.  The test is passing on central…

Comment 32

[Venkatesh (:vpit3833)]

(In reply to Andreas Tolfsen 「:ato」 from comment #31)

> Try looking at what Firefox writes to stdout with the "--gecko-log -"
> argument.
> 

I looked up ./mach run -h, ./mach test -h, and did not find --gecko-log listed in them.  Where can I find it?

> > And, where is this patch pasted here to be applied? on the bookmark for this
> > bug?  mozilla-central?
> 
> It was applied on top your patch.  The test is passing on central…

I applied this fix on top of my bookmark for this bug, did ./mach test testing/firefox-ui/tests/puppeteer/test_places.py -z and still get the same error from Python.  I am still trying to understand the connection between applying the fix on my patch and test passing on central.  Feels as if I am missing something obvious here.  Please explain.

Comment 33

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

(In reply to Venkatesh from comment #32)
> I looked up ./mach run -h, ./mach test -h, and did not find --gecko-log
> listed in them.  Where can I find it?

This option is specific to Marionette harness tests, so it will not be listed by `mach test`. Use at least `mach marionette-test` to see it in the output. 

> I applied this fix on top of my bookmark for this bug, did ./mach test
> testing/firefox-ui/tests/puppeteer/test_places.py -z and still get the same
> error from Python.  I am still trying to understand the connection between

Correct, because you didn't run with `mach firefox-ui-functional ...` but still with `mach test` which is broken as Andreas mentioned in comment 28 about 4 days ago. So please run this command instead.

Comment 34

[Andreas Tolfsen ❲:ato❳]

(In reply to Venkatesh from comment #32)

> I looked up ./mach run -h, ./mach test -h, and did not find
> --gecko-log listed in them.  Where can I find it?

I already mentioned this in
https://bugzilla.mozilla.org/show_bug.cgi?id=1452490#c28.  Just if
there’s any confusion, the full command you need for running this
test and seeing the Firefox stdout is this:

> % ./mach firefox-ui-functional -z --gecko-log -
> testing/firefox-ui/tests/puppeteer/test_places.py

> I applied this fix on top of my bookmark for this bug, did ./mach
> test testing/firefox-ui/tests/puppeteer/test_places.py -z and
> still get the same error from Python.  I am still trying to
> understand the connection between applying the fix on my patch
> and test passing on central.  Feels as if I am missing something
> obvious here.  Please explain.

That is strange.  I’m sure the patch I quoted fixes it.  Maybe the
stdout from Firefox you will get when you run the correct command
will give you an error that is actionable?

Comment 35

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

Comment on attachment 8984507 [details]
Bug 1452490 - Remove marionetteScriptFinished.

https://reviewboard.mozilla.org/r/250358/#review259680

Removing review flag for now until the failures have been fixed.

Comment 36

[Venkatesh (:vpit3833)]

Comment on attachment 8984507 [details]
Bug 1452490 - Remove marionetteScriptFinished.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250358/diff/3-4/

Comment 37

[Venkatesh (:vpit3833)]

I am submitting to review after editing to fix the errors that were thrown by the try server tests previous time.

On both central and local bookmark for this bug, ./mach clobber && ./mach build && ./mach firefox-ui-functional -z returns the following error message on my PC.

FAIL testing/firefox-ui/tests/functional/safebrowsing/test_initial_download.py TestSafeBrowsingInitialDownload.test_safe_browsing_initial_download - AssertionError: 'goog-badbinurl-proto.pset' not found in []
Traceback (most recent call last):
  File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 159, in run
    testMethod()
  File "/home/venkatesh/src/mozilla-central/testing/firefox-ui/tests/functional/safebrowsing/test_initial_download.py", line 111, in test_safe_browsing_initial_download
    self.assertIn(f, files_on_disk_google4)


What can I do here?

Comment 38

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

That failure is unrelated to your changes and tracked in bug 1385613. It would still be nice to find out why this is happening, so if you are interested to help let us know on that other bug after Francois gave me an answer. Thanks.

Comment 39

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

Comment on attachment 8984507 [details]
Bug 1452490 - Remove marionetteScriptFinished.

https://reviewboard.mozilla.org/r/250358/#review260270

It's coming along nicely. Thank you for the update. There are still remaining items to fix, so please see my inline comments.

Further I pushed a new try build so we can check the current status.

::: testing/marionette/evaluate.js:45
(Diff revision 4)
>   *
>   * The arguments provided by the `args<` argument are exposed
>   * through the `arguments` object available in the script context,
>   * and if the script is executed asynchronously with the `async`
>   * option, an additional last argument that is synonymous to the
> - * `marionetteScriptFinished` global is appended, and can be accessed
> + * `????????` global is appended, and can be accessed

This needs an update. Use a wording which matches the `resolve callback`.

::: testing/marionette/evaluate.js:53
(Diff revision 4)
>   * The `timeout` option specifies the duration for how long the
>   * script should be allowed to run before it is interrupted and aborted.
>   * An interrupted script will cause a {@link ScriptTimeoutError} to occur.
>   *
>   * The `async` option indicates that the script will not return
> - * until the `marionetteScriptFinished` global callback is invoked,
> + * until the `????????` global callback is invoked,

Here too. `until the "resolve" callback is invoked`.

::: testing/marionette/evaluate.js
(Diff revision 4)
>  
>      src += `(function() { ${script} }).apply(null, ${ARGUMENTS})`;
>  
> -    // marionetteScriptFinished is not WebDriver conformant,
> -    // hence it is only exposed to immutable sandboxes
> -    if (sandboxName) {

Removing that also removes any usage of `sandboxName` which then causes the ES linter to fail:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0bca58f77baa0a205559dfadab7cbe27f1fc771&selectedJob=183204754

Comment 40

[Venkatesh (:vpit3833)]

Attachment: Bug 1452490 - Remove marionetteScriptFinished.

Review commit: https://reviewboard.mozilla.org/r/254120/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254120/

Comment 41

[Venkatesh (:vpit3833)]

Comment on attachment 8984507 [details]
Bug 1452490 - Remove marionetteScriptFinished.

https://reviewboard.mozilla.org/r/250358/#review260270

> Removing that also removes any usage of `sandboxName` which then causes the ES linter to fail:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0bca58f77baa0a205559dfadab7cbe27f1fc771&selectedJob=183204754

Removed sandboxName entirely.  For ./mach firefox-ui-functional -z, only

FAIL testing/firefox-ui/tests/functional/safebrowsing/test_initial_download.py TestSafeBrowsingInitialDownload.test_safe_browsing_initial_download - AssertionError: 'goog-badbinurl-proto.pset' not found in []

from previous time fails.  No new fails.

Also, ./mach lint passes everything.

Comment 42

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

Thank you for the update. I triggered a new try build. If it passes please set the review flag for me.

Comment 43

[Andreas Tolfsen ❲:ato❳]

It looks like the try run passes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=117b3fdda1ac4c2b5b5d57f98f97d6b25ea548af

But I’m not able to set the review flag because mozreview is, well, terrible.

Comment 44

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

(In reply to Andreas Tolfsen 「:ato」 from comment #43)
> But I’m not able to set the review flag because mozreview is, well, terrible.

You cannot set it because there is a second mozreview request now on this bug, and there haven't been all drafts been published. And this can only be done by the patch author.

Comment 45

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

Oh, and I wish you would have kept the old mozreview request for easier reviewing. Now we have to go through everything again.

Comment 46

[Venkatesh (:vpit3833)]

Sorry for the hold up.  I keep forgetting that there is a publish button.  Not again.

Comment 47

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

Comment on attachment 8984507 [details]
Bug 1452490 - Remove marionetteScriptFinished.

https://reviewboard.mozilla.org/r/250358/#review260270

> Removed sandboxName entirely.  For ./mach firefox-ui-functional -z, only
> 
> FAIL testing/firefox-ui/tests/functional/safebrowsing/test_initial_download.py TestSafeBrowsingInitialDownload.test_safe_browsing_initial_download - AssertionError: 'goog-badbinurl-proto.pset' not found in []
> 
> from previous time fails.  No new fails.
> 
> Also, ./mach lint passes everything.

This sounds good. Thanks.

Comment 48

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

Comment on attachment 8984507 [details]
Bug 1452490 - Remove marionetteScriptFinished.

So this patch is actually not obsolete, but you forgot to amend your changes and as such there are two separate commits again. Please fix that.

Comment 49

[Venkatesh (:vpit3833)]

Comment on attachment 8984507 [details]
Bug 1452490 - Remove marionetteScriptFinished.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250358/diff/4-5/

Comment 50

[Venkatesh (:vpit3833)]

I pushed after histedit/amend.  On review board, there is a r-.  I am not able to set it to r?whimboo.

Comment 51

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

(In reply to Venkatesh (:vpit3833) from comment #50)
> I pushed after histedit/amend.  On review board, there is a r-.  I am not
> able to set it to r?whimboo.

Once the reviewer is set in mozreview there is no need to set it again. Whenever you upload a new version of the patch, a new review request will be automatically send out.

Comment 52

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

Comment on attachment 8984507 [details]
Bug 1452490 - Remove marionetteScriptFinished.

https://reviewboard.mozilla.org/r/250358/#review261830

This looks good and the tests are passing which is great! The remaining issues are mostly cosmetic but for the following I would like to have the feedback from Andreas too:

Personally I would like to see the usage of `let [resolve] = arguments;` across the files, and not `arguments[arguments.length - 1]`. First it's shorter and when multiple args are specified it makes it clear what's all in the argument list because you have to destruct everything.

::: browser/components/migration/tests/marionette/test_refresh_firefox.py:99
(Diff revision 5)
>              fieldname: arguments[0],
>              value: arguments[1],
>              firstUsed: (Date.now() - 5000) * 1000,
>            };
>            let finished = false;
> +          let resolve = arguments[arguments.length - 1];

Please move this up to the first line of the code.

::: browser/components/migration/tests/marionette/test_refresh_firefox.py:189
(Diff revision 5)
>      def createSync(self):
>          # This script will write an entry to the login manager and create
>          # a signedInUser.json in the profile dir.
>          self.runAsyncCode("""
>            Cu.import("resource://gre/modules/FxAccountsStorage.jsm");
> +          let resolve = arguments[arguments.length - 1];

Please move this a line up so that getting arguments is always first.

::: browser/components/migration/tests/marionette/test_refresh_firefox.py:216
(Diff revision 5)
>          # Note that we expect 2 logins - one from us, one from sync.
>          self.assertEqual(loginCount, 2, "No other logins are present")
>  
>      def checkBookmarkInMenu(self):
>          titleInBookmarks = self.runAsyncCode("""
> +          let resolve = arguments[arguments.length - 1];

This should come after `url`.

::: browser/components/migration/tests/marionette/test_refresh_firefox.py:282
(Diff revision 5)
>              self.assertEqual(
>                  formFieldResults[0]['value'], self._formHistoryValue)
>  
>          formHistoryCount = self.runAsyncCode("""
>            let count;
> +          let resolve = arguments[arguments.length - 1];

Move up please.

::: testing/awsy/awsy/awsy_test_case.py:151
(Diff revision 5)
>          gc_script = """
>              Cu.import("resource://gre/modules/Services.jsm");
>              Services.obs.notifyObservers(null, "child-mmu-request", null);
>  
>              let memMgrSvc = Cc["@mozilla.org/memory-reporter-manager;1"].getService(Ci.nsIMemoryReporterManager);
> -            memMgrSvc.minimizeMemoryUsage(() => marionetteScriptFinished("gc done!"));
> +            memMgrSvc.minimizeMemoryUsage(() => {arguments[0]("gc done!");});

Lets add the `resolve` variable for all the instances in this file please.

::: testing/marionette/client/docs/basics.rst:171
(Diff revision 5)
>  .. parsed-literal::
>     result = client.execute_script("return arguments[0] + arguments[1];",
>                                    script_args=[2, 3])
>     assert result == 5
>  
>  The async method works the same way, except it won't return until a special

I would remove `special` now and call it `until the 'resolve' function is called`.

::: testing/marionette/client/docs/basics.rst:176
(Diff revision 5)
>  The async method works the same way, except it won't return until a special
> -`marionetteScriptFinished()` function is called:
> +`resolve()` function is called:
>  
>  .. parsed-literal::
>     result = client.execute_async_script("""
> +       let [resolve] = arguments;

Personally I like this more and would like to see it everywhere.

::: testing/marionette/harness/marionette_harness/tests/unit/test_addons.py:34
(Diff revision 5)
>      @property
>      def all_addon_ids(self):
>          with self.marionette.using_context("chrome"):
>              addons = self.marionette.execute_async_script("""
>                Components.utils.import("resource://gre/modules/AddonManager.jsm");
> +              let [resolve] = arguments;

Please move this up, and maybe add an empty line to the import.

::: testing/marionette/harness/marionette_harness/tests/unit/test_addons.py:48
(Diff revision 5)
>      def reset_addons(self):
>          with self.marionette.using_context("chrome"):
>              for addon in (self.all_addon_ids - self.preinstalled_addons):
>                  addon_id = self.marionette.execute_async_script("""
>                    Components.utils.import("resource://gre/modules/AddonManager.jsm");
> +                  let [resolve] = arguments;

Same here.

::: testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py
(Diff revision 5)
>  
>      def test_execute_async_js_exception(self):
>          try:
>              self.marionette.execute_async_script("""
> -                let [resolve] = arguments;
> +                arguments[0](foo());
> -                resolve(foo());

Please leave that as is.

::: testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py:102
(Diff revision 5)
>          self.assertEqual(self.marionette.execute_async_script(
> -            "marionetteScriptFinished(this.foobar);", new_sandbox=False), [23, 42])
> +            "arguments[0](this.foobar);", new_sandbox=False), [23, 42])
>  
>      def test_sandbox_refresh_arguments(self):
>          self.marionette.execute_async_script("this.foobar = [arguments[0], arguments[1]];"
> -                                             "marionetteScriptFinished();",
> +                                             "let resolve = arguments[arguments.length - 1];"

The line is getting way long, better to break the arguments into a new line with 4-chars of indentation.

::: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py:52
(Diff revision 5)
>          """
>          return self.marionette.execute_async_script("""
>            Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
>  
> -          let folderGuids = []
> +          let folderGuids = [];
> +          let [url, resolve] = arguments;

Please move up as first line of the script code.

::: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py:79
(Diff revision 5)
>      def restore_default_bookmarks(self):
>          """Restore the default bookmarks for the current profile."""
>          retval = self.marionette.execute_async_script("""
>            Components.utils.import("resource://gre/modules/BookmarkHTMLUtils.jsm");
>  
> +          let [resolve] = arguments;

Same here.

::: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py:120
(Diff revision 5)
>      def remove_all_history(self):
>          """Remove all history items."""
>          retval = self.marionette.execute_async_script("""
>              Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
>  
> +            let [resolve] = arguments;

And here.

::: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/software_update.py:386
(Diff revision 5)
>          :returns: The URL of the update snippet
>          """
>          url = self.marionette.execute_async_script("""
>            Components.utils.import("resource://gre/modules/UpdateUtils.jsm");
>            let res = UpdateUtils.formatUpdateURL(arguments[0]);
> +          let resolve = arguments[arguments.length - 1];

And here.

::: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/utils.py:57
(Diff revision 5)
>          """
>  
>          with self.marionette.using_context('chrome'):
>              result = self.marionette.execute_async_script("""
>                var {Sanitizer} = Components.utils.import("resource:///modules/Sanitizer.jsm", {});
> +              let resolve = arguments[arguments.length - 1];

And here.

Comment 53

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

(In reply to Henrik Skupin (:whimboo) [away 07/08 - 07/22] from comment #52)
> Personally I would like to see the usage of `let [resolve] = arguments;`
> across the files, and not `arguments[arguments.length - 1]`. First it's
> shorter and when multiple args are specified it makes it clear what's all in
> the argument list because you have to destruct everything.

Andreas, what do you think?

Comment 54

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) [away 07/08 - 07/22] from
comment #53)

> (In reply to Henrik Skupin (:whimboo) [away 07/08 - 07/22] from
> comment #52)
> 
> > Personally I would like to see the usage of `let
> > [resolve] = arguments;` across the files, and not
> > `arguments[arguments.length - 1]`. First it's shorter and when
> > multiple args are specified it makes it clear what's all in the
> > argument list because you have to destruct everything.
> 
> Andreas, what do you think?

You can’t always do this if there is more than one argument.

If there are three arguments and you would have to expand all four
arguments even if you don’t plan on using them, since there is no
way to dismiss unused variables in JS:

> let [foo, bar, baz, resolve] = arguments;

One a more general note I don’t feel that the style used for getting
at the callback argument is the most important thing to fret over.
If the patch fulfills the goal of removing marionetteScriptFinished,
that moves us into a position of removing this from the Marionette
server, which is of greater value than the style of our internal
tests.

Comment 55

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

(In reply to Andreas Tolfsen 「:ato」 from comment #54)
> You can’t always do this if there is more than one argument.
> 
> If there are three arguments and you would have to expand all four
> arguments even if you don’t plan on using them, since there is no
> way to dismiss unused variables in JS:
> 
> > let [foo, bar, baz, resolve] = arguments;

Sure, in such a case it is totally fine to index the arguments array. But in most of the cases you will also make use of the arguments which are getting passed-in. 

> One a more general note I don’t feel that the style used for getting
> at the callback argument is the most important thing to fret over.
> If the patch fulfills the goal of removing marionetteScriptFinished,
> that moves us into a position of removing this from the Marionette
> server, which is of greater value than the style of our internal
> tests.

I will go through the open issue, and close those which aren't necessary.

For the rest I would like that you do the final review.

Comment 56

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

Oh, and I actually meant to give a r+ with the changes made. I will also correct that.

Comment 57

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

Comment on attachment 8984507 [details]
Bug 1452490 - Remove marionetteScriptFinished.

https://reviewboard.mozilla.org/r/250358/#review261830

> Please move this a line up so that getting arguments is always first.

You can also use `let [resolve] = arguments;` here.

> This should come after `url`.

Even better use destructuring here: `let [url, resolve] = arguments;`.

> Move up please.

Also use destructuring here please.

Comment 58

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

Comment on attachment 8984507 [details]
Bug 1452490 - Remove marionetteScriptFinished.

https://reviewboard.mozilla.org/r/250358/#review261860

I won't be around the next two weeks. So please request an additional review if necessary from Andreas. Thanks.

Comment 59

[Venkatesh (:vpit3833)]

Comment on attachment 8984507 [details]
Bug 1452490 - Remove marionetteScriptFinished.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250358/diff/5-6/

Comment 60

[Venkatesh (:vpit3833)]

Is this better?

Comment 61

[Andreas Tolfsen ❲:ato❳]

It’s hard for me to tell what’s been fixed since there are 14 open
issues on the review (mainly because mozreview doesn’t really present
this in an intuitive way).  Can you please close the issues you
have fixed?

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d95faa57e12e73c7c302e48a9f429c138144e200

Comment 62

[Venkatesh (:vpit3833)]

Fixed all the issues that were left open.  Closed them all now.

Comment 63

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8eab40c27903
Remove marionetteScriptFinished. r=whimboo

Comment 64

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/8eab40c27903

Comment 65

[Andreas Tolfsen ❲:ato❳]

Thanks for fixing this Venkatesh!  I got your privmsg on IRC, but
was on holiday yesterday.