Closed Bug 1458119 Opened 6 years ago Closed 6 years ago

Automated test for shutdown/restore with Restart Manager fails in debug

Categories

(Firefox :: Session Restore, defect, P2)

Product:
Firefox
Component:
Session Restore
Platform:
Unspecified
Windows
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: agashlin, Assigned: agashlin)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1458119: Part 1: Use idleDispatch to be independent from a content window.
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 1458119: Part 2: Test session restore across Windows shutdown.
59 bytes, text/x-review-board-request
Gijs
: review+
whimboo
: review+
Details 
I had intended to include a test using a synthesized WM_ENDSESSION in bug 603903, but the test I came up with is failing much of the time in debug builds (not all tabs restored). I'd like to figure out if the test is wrong or possibly what bugs it is exposing in session save/restore.

This is a variation on test_restore_windows_after_restart_and_quit [1], using ctypes to send the WM_QUERYENDSESSION and WM_ENDSESSION messages to the top-level windows of the process.

I had to disable setting browser.sessionstore.debug.no_auto_updates or there was much more failure even with a single window, currently it uses three windows as in the original test_restore_windows_after_restart_and_quit.

I'm running it as:
./mach firefox-ui-functional testing/firefox-ui/tests/functional/sessionstore/test_restore_after_endsession.py

I wasn't sure whether this should block 1270666, or 212316, to let interested parties know about this work in progress; I've erred on the side of not bugging too many people for now.

Reviewboard attachment to follow shortly.

[1] https://searchfox.org/mozilla-central/rev/8837610b6c999451435695e800f38d4acbc0a644/testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py
Comment on attachment 8972204 [details]
Bug 1458119: Test shutdown/restore with WM_ENDSESSION

https://reviewboard.mozilla.org/r/240868/#review246646


Code analysis found 7 defects in this patch:
 - 7 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/firefox-ui/tests/functional/sessionstore/test_restore_after_endsession.py:9
(Diff revision 1)
> +
> +from firefox_puppeteer import PuppeteerMixin
> +from marionette_harness import MarionetteTestCase
> +import ctypes
> +import ctypes.wintypes
> +import time

Error: 'time' imported but unused [flake8: F401]

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_after_endsession.py:12
(Diff revision 1)
> +import ctypes
> +import ctypes.wintypes
> +import time
> +
> +# TestBaseRestoreWindows is copied from test_restore_windows_after_restart_and_quit
> +class TestBaseRestoreWindows(PuppeteerMixin, MarionetteTestCase):

Error: Expected 2 blank lines, found 1 [flake8: E302]

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_after_endsession.py:52
(Diff revision 1)
> +            # the user to click on the background tabs
> +            'browser.sessionstore.restore_on_demand': False,
> +            # Avoid race conditions by having the content process never
> +            # send us session updates unless the parent has explicitly asked
> +            # for them via the TabStateFlusher.
> +            #'browser.sessionstore.debug.no_auto_updates': True,

Error: Block comment should start with '# ' [flake8: E265]

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_after_endsession.py:150
(Diff revision 1)
> +            for tab in win.tabbar.tabs:
> +                urls = urls + tuple([tab.location])
> +            opened_windows.add(urls)
> +        return opened_windows
> +
> +def wm_end_session(pid):

Error: Expected 2 blank lines, found 1 [flake8: E302]

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_after_endsession.py:188
(Diff revision 1)
> +
> +    def send_message_endsession(hWnd):
> +        SendMessage(hWnd, WM_ENDSESSION, True, ENDSESSION_CLOSEAPP)
> +
> +    send_message = None
> +    def cb_enum(hWnd, lParam):

Error: Expected 1 blank line before a nested definition, found 0 [flake8: E306]

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_after_endsession.py:191
(Diff revision 1)
> +
> +    send_message = None
> +    def cb_enum(hWnd, lParam):
> +        owner = GetWindow(hWnd, GW_OWNER)
> +        # only look at top-level (ownerless) windows
> +        if owner == None:

Error: Comparison to none should be 'if cond is none:' [flake8: E711]

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_after_endsession.py:240
(Diff revision 1)
> +        current_windows_set = self.convert_open_windows_to_set()
> +        self.assertEqual(current_windows_set, self.test_windows,
> +                         msg="""Non private browsing windows should have
> +                         been restored. Expected {}, got {}.
> +                         """.format(self.test_windows, current_windows_set))
> +

Error: Blank line at end of file [flake8: W391]
Attachment #8972204 - Attachment is obsolete: true
Mike, if you get a chance could you take a look at the results of running this test with a debug build? I don't really know how to interpret what I'm seeing, some tabs are restored as about:newtab. Is the slowness of a debug build exposing something asynchronous not finishing during the synchronous shutdown?
Flags: needinfo?(mdeboer)
Summary: Test for WM_ENDSESSION → Automated test for WM_ENDSESSION fails in debug
Blocks: ss-reliability, 603903
Priority: -- → P2
Assignee: nobody → agashlin
Status: NEW → ASSIGNED
Flags: needinfo?(mdeboer)
In this new test I'm explicitly using the Restart Manager to shut down the process, I think this is a better reflection of what Windows does than the hack using WM_QUERYENDSESSION and WM_ENDSESSION. In particular, this way Windows doesn't send the application any messages after WM_ENDSESSION, which might have been causing issues.

Still seeing the same failures, though they might be a little more rare now.
Summary: Automated test for WM_ENDSESSION fails in debug → Automated test for shutdown/restore with Restart Manager fails in debug
I think I've found the cause: In MessageQueue.sendWhenIdle we use content.requestIdleCallback to schedule sending [1], but if things are running sufficiently slowly the content can go away before our callback is hit. I've speculatively fixed this by instead using ChromeUtils.idleDispatch, which has no such reliance on a content window. It seems to be testing alright but it'll need a close look as this is mostly foreign to me.

[1] https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/browser/components/sessionstore/content/content-sessionStore.js#858
Comment on attachment 8972210 [details]
Bug 1458119: Part 1: Use idleDispatch to be independent from a content window.

https://reviewboard.mozilla.org/r/240874/#review251680


Code analysis found 10 defects in this patch:
 - 10 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_windows_shutdown.py:246
(Diff revision 3)
> +    sessionKey = sessionKeyType()
> +    if RmStartSession(pointer(dwSessionHandle), 0, sessionKey) != ERROR_SUCCESS:
> +        return False
> +
> +    try:
> +      # Register the process as needing to be shut down

Error: Indentation is not a multiple of four (comment) [flake8: E114]

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_windows_shutdown.py:247
(Diff revision 3)
> +    if RmStartSession(pointer(dwSessionHandle), 0, sessionKey) != ERROR_SUCCESS:
> +        return False
> +
> +    try:
> +      # Register the process as needing to be shut down
> +      if RmRegisterResources(dwSessionHandle, 0, None, 1, UProcs, 0, None) != ERROR_SUCCESS:

Error: Indentation is not a multiple of four [flake8: E111]

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_windows_shutdown.py:248
(Diff revision 3)
> +        return False
> +
> +    try:
> +      # Register the process as needing to be shut down
> +      if RmRegisterResources(dwSessionHandle, 0, None, 1, UProcs, 0, None) != ERROR_SUCCESS:
> +          return False

Error: Indentation is not a multiple of four [flake8: E111]

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_windows_shutdown.py:250
(Diff revision 3)
> +    try:
> +      # Register the process as needing to be shut down
> +      if RmRegisterResources(dwSessionHandle, 0, None, 1, UProcs, 0, None) != ERROR_SUCCESS:
> +          return False
> +
> +      # Shut down all processes using registered resources

Error: Indentation is not a multiple of four (comment) [flake8: E114]

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_windows_shutdown.py:251
(Diff revision 3)
> +      # Register the process as needing to be shut down
> +      if RmRegisterResources(dwSessionHandle, 0, None, 1, UProcs, 0, None) != ERROR_SUCCESS:
> +          return False
> +
> +      # Shut down all processes using registered resources
> +      if RmShutdown(dwSessionHandle, 0, ctypes.cast(None, RM_WRITE_STATUS_CALLBACK)) != ERROR_SUCCESS:

Error: Indentation is not a multiple of four [flake8: E111]

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_windows_shutdown.py:251
(Diff revision 3)
> +      # Register the process as needing to be shut down
> +      if RmRegisterResources(dwSessionHandle, 0, None, 1, UProcs, 0, None) != ERROR_SUCCESS:
> +          return False
> +
> +      # Shut down all processes using registered resources
> +      if RmShutdown(dwSessionHandle, 0, ctypes.cast(None, RM_WRITE_STATUS_CALLBACK)) != ERROR_SUCCESS:

Error: Line too long (102 > 99 characters) [flake8: E501]

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_windows_shutdown.py:252
(Diff revision 3)
> +      if RmRegisterResources(dwSessionHandle, 0, None, 1, UProcs, 0, None) != ERROR_SUCCESS:
> +          return False
> +
> +      # Shut down all processes using registered resources
> +      if RmShutdown(dwSessionHandle, 0, ctypes.cast(None, RM_WRITE_STATUS_CALLBACK)) != ERROR_SUCCESS:
> +          return False

Error: Indentation is not a multiple of four [flake8: E111]

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_windows_shutdown.py:254
(Diff revision 3)
> +
> +      # Shut down all processes using registered resources
> +      if RmShutdown(dwSessionHandle, 0, ctypes.cast(None, RM_WRITE_STATUS_CALLBACK)) != ERROR_SUCCESS:
> +          return False
> +
> +      return True

Error: Indentation is not a multiple of four [flake8: E111]

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_windows_shutdown.py:257
(Diff revision 3)
> +          return False
> +
> +      return True
> +
> +    finally:
> +      RmEndSession(dwSessionHandle)

Error: Indentation is not a multiple of four [flake8: E111]

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_windows_shutdown.py:259
(Diff revision 3)
> +      return True
> +
> +    finally:
> +      RmEndSession(dwSessionHandle)
> +
> +class TestSessionStoreShutdown(TestBaseRestoreWindows):

Error: Expected 2 blank lines, found 1 [flake8: E302]
Comment on attachment 8972210 [details]
Bug 1458119: Part 1: Use idleDispatch to be independent from a content window.

https://reviewboard.mozilla.org/r/240874/#review251852

r=me on the change to ChromeUtils, but can you split that change off to its own commit?
The test file commit should then be reviewed by Henrik [:whimboo], instead of me.

Thanks!
Attachment #8972210 - Flags: review?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #11)
> Comment on attachment 8972210 [details]
> r=me on the change to ChromeUtils, but can you split that change off to its
> own commit?
> The test file commit should then be reviewed by Henrik [:whimboo], instead
> of me.
> 
> Thanks!

Thanks Mike, will do!
Comment on attachment 8972210 [details]
Bug 1458119: Part 1: Use idleDispatch to be independent from a content window.

https://reviewboard.mozilla.org/r/240874/#review252258

::: browser/components/sessionstore/content/content-sessionStore.js:858
(Diff revision 5)
>      } else if (MessageQueue._idleCallbackID) {
>        // Bail out if there's a pending run.
>        return;
>      }
>      MessageQueue._idleCallbackID =
> -      content.requestIdleCallback(MessageQueue.sendWhenIdle, {timeout: MessageQueue._timeoutWaitIdlePeriodMs});
> +      ChromeUtils.idleDispatch(MessageQueue.sendWhenIdle, {timeout: MessageQueue._timeoutWaitIdlePeriodMs});

Upon better inspection, I noticed that `ChromeUtils.idleDispatch` doesn't return an ID, so we should keep track of this more as you can see at https://searchfox.org/mozilla-central/source/toolkit/components/extensions/MessageChannel.jsm#297

It'll be a small change, but quite an important one.
Attachment #8972210 - Flags: review?(mdeboer) → review-
Comment on attachment 8972210 [details]
Bug 1458119: Part 1: Use idleDispatch to be independent from a content window.

https://reviewboard.mozilla.org/r/240874/#review252258

> Upon better inspection, I noticed that `ChromeUtils.idleDispatch` doesn't return an ID, so we should keep track of this more as you can see at https://searchfox.org/mozilla-central/source/toolkit/components/extensions/MessageChannel.jsm#297
> 
> It'll be a small change, but quite an important one.

Ouch, that's a good catch, everything being done with \_idleCallbackID became nonsense with that change. I think what I've put in to make a fake, cancellable handle should work now.
Comment on attachment 8979688 [details]
Bug 1458119: Part 2: Test session restore across Windows shutdown.

https://reviewboard.mozilla.org/r/245832/#review252462

::: commit-message-582db:2
(Diff revision 3)
> +Bug 1458119: Part 2: Test session restore across Windows shutdown. r?whimboo
> +

Why do we limit this to Windows only? If that is true please add a more descriptive comment to the commit message.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py:6
(Diff revision 3)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  from firefox_puppeteer import PuppeteerMixin
> -from marionette_harness import MarionetteTestCase
> +from marionette_harness import MarionetteTestCase, run_if_os_win

No need to add a new decorator here. Just use something like that in the test instead:

> if self.marionette.session_capabilities["platformName"] != "windows_nt":
>     skip("Bug XYZ - description")

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py:205
(Diff revision 3)
>                           msg="""Non private browsing windows should have
>                           been restored. Expected {}, got {}.
>                           """.format(self.test_windows, current_windows_set))
> +
> +
> +def shutdown_with_restart_manager(pid):

Can you please explain what you are trying to do here? I miss the complete context, and anyone else will not be able too to understand this method.

Also why does a normal `self.marionette.quit()` or `self.marionette.restart()` not work?

This kills the process, and starts a new one. So the behavior should be the same, right?

If we really have to keep the ctypes code I would propose to move out this test to its own file.
Attachment #8979688 - Flags: review?(hskupin)
Comment on attachment 8979688 [details]
Bug 1458119: Part 2: Test session restore across Windows shutdown.

https://reviewboard.mozilla.org/r/245832/#review252462

Thanks Henrik, you're right that I need a lot more documentation here.

> No need to add a new decorator here. Just use something like that in the test instead:
> 
> > if self.marionette.session_capabilities["platformName"] != "windows_nt":
> >     skip("Bug XYZ - description")

That's much nicer, thanks!

> Can you please explain what you are trying to do here? I miss the complete context, and anyone else will not be able too to understand this method.
> 
> Also why does a normal `self.marionette.quit()` or `self.marionette.restart()` not work?
> 
> This kills the process, and starts a new one. So the behavior should be the same, right?
> 
> If we really have to keep the ctypes code I would propose to move out this test to its own file.

I'll add a proper docstring for this shortly.

This is intended to test the [Windows synchronous shutdown](https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/widget/windows/nsWindow.cpp#5248), where we have to be careful to have everything finished before we're done handling WM_ENDSESSION. The best way to simulate this is with the Restart Manager as I use it here. I'll mention this in the documentation, and I agree that it would be better off in another file.
Comment on attachment 8979688 [details]
Bug 1458119: Part 2: Test session restore across Windows shutdown.

https://reviewboard.mozilla.org/r/245832/#review252462

> That's much nicer, thanks!

When you move it to a different file, can can set the skip flag already in the manifest file which makes it even more easier.

> I'll add a proper docstring for this shortly.
> 
> This is intended to test the [Windows synchronous shutdown](https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/widget/windows/nsWindow.cpp#5248), where we have to be careful to have everything finished before we're done handling WM_ENDSESSION. The best way to simulate this is with the Restart Manager as I use it here. I'll mention this in the documentation, and I agree that it would be better off in another file.

Ok, I will wait with further comments then. Thanks.
Comment on attachment 8979688 [details]
Bug 1458119: Part 2: Test session restore across Windows shutdown.

https://reviewboard.mozilla.org/r/245832/#review252462

> When you move it to a different file, can can set the skip flag already in the manifest file which makes it even more easier.

Would it be reasonable to copy the `TestBaseRestoreWindows` class from the current test file? Or how would you suggest that I go about sharing it? I don't think I can just `import` it, as this directory isn't in the Python search path.
Comment on attachment 8979688 [details]
Bug 1458119: Part 2: Test session restore across Windows shutdown.

https://reviewboard.mozilla.org/r/245832/#review252462

> Would it be reasonable to copy the `TestBaseRestoreWindows` class from the current test file? Or how would you suggest that I go about sharing it? I don't think I can just `import` it, as this directory isn't in the Python search path.

You could create an empty __init__.py file and do a local import.
Comment on attachment 8979688 [details]
Bug 1458119: Part 2: Test session restore across Windows shutdown.

https://reviewboard.mozilla.org/r/245832/#review252462

> You could create an empty __init__.py file and do a local import.

It should have been `__init__.py` file.
Comment on attachment 8972210 [details]
Bug 1458119: Part 1: Use idleDispatch to be independent from a content window.

https://reviewboard.mozilla.org/r/240874/#review252814

::: browser/components/sessionstore/content/content-sessionStore.js:788
(Diff revision 8)
>    /**
>     * Cleanup pending idle callback and timer.
>     */
>    cleanupTimers() {
> -    if (this._idleCallbackID) {
> -      content.cancelIdleCallback(this._idleCallbackID);
> +    if (this._idleHandle) {
> +      this._idleHandle.cancelled = true;

This won't do what you're expecting, I think. So let's approach this from a different angle:

Let's ditch the cancel-ability of the dispatch-send-when-idle functionality and make it fire-and-forget instead. In other words; make it work _just_ like at https://searchfox.org/mozilla-central/source/toolkit/components/extensions/MessageChannel.jsm#297:

```js
} else if (MessageQueue._idleScheduled) {
  // Bail out if there is a pending run.
  return;
}

ChromeUtils.idleDispatch(this.sendWhenIdle, {timeout:
MessageQueue._timeoutWaitIdlePeriodMs});
MessageQueue._idleScheduled = true;
```

And do `MessageQueue._idleScheduled = false;` in `cleanupTimers`.
Attachment #8972210 - Flags: review?(mdeboer) → review-
Comment on attachment 8972210 [details]
Bug 1458119: Part 1: Use idleDispatch to be independent from a content window.

https://reviewboard.mozilla.org/r/240874/#review252814

> This won't do what you're expecting, I think. So let's approach this from a different angle:
> 
> Let's ditch the cancel-ability of the dispatch-send-when-idle functionality and make it fire-and-forget instead. In other words; make it work _just_ like at https://searchfox.org/mozilla-central/source/toolkit/components/extensions/MessageChannel.jsm#297:
> 
> ```js
> } else if (MessageQueue._idleScheduled) {
>   // Bail out if there is a pending run.
>   return;
> }
> 
> ChromeUtils.idleDispatch(this.sendWhenIdle, {timeout:
> MessageQueue._timeoutWaitIdlePeriodMs});
> MessageQueue._idleScheduled = true;
> ```
> 
> And do `MessageQueue._idleScheduled = false;` in `cleanupTimers`.

Ok, I can see how we don't really need cancellation, I'll leave it as a simple scheduled flag. Thanks for your patience. If there was a bug with the last attempt, could you point it out? Or was it perhaps just overcomplicated?
Comment on attachment 8972210 [details]
Bug 1458119: Part 1: Use idleDispatch to be independent from a content window.

https://reviewboard.mozilla.org/r/240874/#review253488

Looks great to me, thanks!
Attachment #8972210 - Flags: review?(mdeboer) → review+
Comment on attachment 8972210 [details]
Bug 1458119: Part 1: Use idleDispatch to be independent from a content window.

https://reviewboard.mozilla.org/r/240874/#review252814

> Ok, I can see how we don't really need cancellation, I'll leave it as a simple scheduled flag. Thanks for your patience. If there was a bug with the last attempt, could you point it out? Or was it perhaps just overcomplicated?

Sorry, I didn't explain well what the issue was indeed :-P

In `cleanupTimers`, you do:
```js
this._idleHandle.cancelled = true;
this._idleHandle = null;
```
... whereas the check for `_idleHandle.cancelled` is in the `idleDispatch` callback. But the problem here is that you explicitly set `_idleHandle` to `null`, which means that the `cancelled` flag is not available anymore.
This leaves you with no reliable way to cancel the callback.
Comment on attachment 8972210 [details]
Bug 1458119: Part 1: Use idleDispatch to be independent from a content window.

https://reviewboard.mozilla.org/r/240874/#review252814

> Sorry, I didn't explain well what the issue was indeed :-P
> 
> In `cleanupTimers`, you do:
> ```js
> this._idleHandle.cancelled = true;
> this._idleHandle = null;
> ```
> ... whereas the check for `_idleHandle.cancelled` is in the `idleDispatch` callback. But the problem here is that you explicitly set `_idleHandle` to `null`, which means that the `cancelled` flag is not available anymore.
> This leaves you with no reliable way to cancel the callback.

I think it still worked, the check in the callback is not referenced through `_idleHandle`, but rather through `handle` from the enclosing scope, which is captured in the closure and will only refer to the object that was created along with the callback. This could have been documented better, clearly.

Thanks again for the review!
Comment on attachment 8979688 [details]
Bug 1458119: Part 2: Test session restore across Windows shutdown.

https://reviewboard.mozilla.org/r/245832/#review253978

Beside the inline comments I'm not that familar with the ctypes code and API usage. So if Mike could also have a look at this code I would appreciate.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart_and_quit.py:9
(Diff revision 5)
>  
>  from firefox_puppeteer import PuppeteerMixin
>  from marionette_harness import MarionetteTestCase
>  
>  
>  class TestBaseRestoreWindows(PuppeteerMixin, MarionetteTestCase):

Please move this class out into it's own file, so it is separated from any test module.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_windows_shutdown.py:12
(Diff revision 5)
> +
> +# Apologies, I don't know of a better way to implement this import.
> +imp.load_source('test_base_restore',
> +                join(dirname(__file__),
> +                     'test_restore_windows_after_restart_and_quit.py'))
> +

An `import .filename` should be fine here.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_windows_shutdown.py:16
(Diff revision 5)
> +                     'test_restore_windows_after_restart_and_quit.py'))
> +
> +from test_base_restore import TestBaseRestoreWindows
> +
> +
> +def shutdown_with_restart_manager(pid):

I would also move this method to the new file you are about to create. Maybe make it even part of the base class and assert at the top that only Windows is supported. Could we extend it to other platforms too in the future, eg. MacOS also reopens when after a restart?

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_windows_shutdown.py:152
(Diff revision 5)
> +
> +    def setUp(self):
> +        super(TestSessionStoreWindowsShutdown, self).setUp(startup_page=3, no_auto_updates=False)
> +
> +    def test_with_variety(self):
> +        """ Opens a set of windows, both standard and private, with

Please add an overview as first line which doesn't exceed 80 chars, and separate the remaining docstring by an empty line.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_windows_shutdown.py:172
(Diff revision 5)
> +
> +        pid = self.marionette.process_id
> +        self.marionette.delete_session()
> +        shutdown_success = shutdown_with_restart_manager(pid)
> +        self.assertTrue(shutdown_success, msg='RmShutdown failed')
> +        self.marionette.instance.runner.wait(timeout=self.marionette.DEFAULT_SHUTDOWN_TIMEOUT)

Instead of manually handling the restart please make use of the `callback` argument for `self.marionette.quit()`. It would look like:

> self.marionette.restart(in_app=True, callback=method)

The callback cannot have an argument, so you want to use lamba here.
Attachment #8979688 - Flags: review?(hskupin) → review-
Comment on attachment 8979688 [details]
Bug 1458119: Part 2: Test session restore across Windows shutdown.

https://reviewboard.mozilla.org/r/245832/#review253978

> An `import .filename` should be fine here.

Unfortunately this doesn't seem to work, I think in part due to [the way test modules are loaded](https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/testing/marionette/harness/marionette_harness/marionette_test/testcases.py#315). Even with `__init__.py` Python doesn't consider the module to be in a package. With:
```python
from .test_restore_windows_after_restart_and_quit import TestBaseRestoreWindows
```
I get `ValueError: Attempted relative import in non-package` (of course the file name will be cleaned up when I moved `TestBaseRestoreWindows` to a common file as you recommend)

And `imp.load_source()` doesn't work with directories, so I can't just import all of `sessionstore`.

The update tests include a lot of base functionality in [`UpdateTestCase`](https://searchfox.org/mozilla-central/source/testing/firefox-ui/harness/firefox_ui_harness/testcases.py#19), would it make sense to drop `TestBaseRestoreWindows` in `testcases.py`?

> I would also move this method to the new file you are about to create. Maybe make it even part of the base class and assert at the top that only Windows is supported. Could we extend it to other platforms too in the future, eg. MacOS also reopens when after a restart?

Yes, I'd like to see this tested for Mac as well, see bug 1326181

> Instead of manually handling the restart please make use of the `callback` argument for `self.marionette.quit()`. It would look like:
> 
> > self.marionette.restart(in_app=True, callback=method)
> 
> The callback cannot have an argument, so you want to use lamba here.

`restart()` or `quit()` didn't seem appropriate here. The browser will be shutting down via `shutdown_with_restart_manager`, so there will be nothing for marionette to communicate with using `in_app=True` (or if there is it will interfere with the shutdown under test), and I don't want `in_app=False` as that will potentially kill the application before it finishes the synchronous shutdown.

I don't like having to poke into marionette like this, but I don't think there's a better option when dealing directly with the application lifecycle as this test has to do.
Comment on attachment 8979688 [details]
Bug 1458119: Part 2: Test session restore across Windows shutdown.

https://reviewboard.mozilla.org/r/245832/#review253978

> Unfortunately this doesn't seem to work, I think in part due to [the way test modules are loaded](https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/testing/marionette/harness/marionette_harness/marionette_test/testcases.py#315). Even with `__init__.py` Python doesn't consider the module to be in a package. With:
> ```python
> from .test_restore_windows_after_restart_and_quit import TestBaseRestoreWindows
> ```
> I get `ValueError: Attempted relative import in non-package` (of course the file name will be cleaned up when I moved `TestBaseRestoreWindows` to a common file as you recommend)
> 
> And `imp.load_source()` doesn't work with directories, so I can't just import all of `sessionstore`.
> 
> The update tests include a lot of base functionality in [`UpdateTestCase`](https://searchfox.org/mozilla-central/source/testing/firefox-ui/harness/firefox_ui_harness/testcases.py#19), would it make sense to drop `TestBaseRestoreWindows` in `testcases.py`?

I see in regards of the local import. I checked the Marionette unit tests and what we do there is:

> # add this directory to the path
> sys.path.append(os.path.dirname(__file__))
>
> from test_switch_window_content import TestSwitchToWindowContent

So this should work too.

You mentioned the UpdateTestCase in your last comment. This is not the appropriate class to use here given that this is only used for update tests. If we wanted a feature like that we would have to get it added to Marionette itself. For now I don't see the benefit yet given that this is the only test which makes use of it.

> Yes, I'd like to see this tested for Mac as well, see bug 1326181

Cool! Thanks

> `restart()` or `quit()` didn't seem appropriate here. The browser will be shutting down via `shutdown_with_restart_manager`, so there will be nothing for marionette to communicate with using `in_app=True` (or if there is it will interfere with the shutdown under test), and I don't want `in_app=False` as that will potentially kill the application before it finishes the synchronous shutdown.
> 
> I don't like having to poke into marionette like this, but I don't think there's a better option when dealing directly with the application lifecycle as this test has to do.

Well, I wrote all the quit and restart logic in Marionette, so I know it will be working. So if you need a quit here as triggered by a callback you have to use `self.marionette.quit(in_app=True, callback=lambda: ...)`. This will allow the callback initiate a shutdown of Firefox with whatever method it wants. Marionette will internally handle everything necessary, which you duplicated here in the test. As example see the Marionette unit test file `test_quit_restart.py`.
Comment on attachment 8979688 [details]
Bug 1458119: Part 2: Test session restore across Windows shutdown.

https://reviewboard.mozilla.org/r/245832/#review253978

> Well, I wrote all the quit and restart logic in Marionette, so I know it will be working. So if you need a quit here as triggered by a callback you have to use `self.marionette.quit(in_app=True, callback=lambda: ...)`. This will allow the callback initiate a shutdown of Firefox with whatever method it wants. Marionette will internally handle everything necessary, which you duplicated here in the test. As example see the Marionette unit test file `test_quit_restart.py`.

Sorry, I jumped to a conclusion about `callback` that was totally wrong, that's what I get for not reading the docs. I'll use `self.marionette.reset`. Thanks again.
Comment on attachment 8979688 [details]
Bug 1458119: Part 2: Test session restore across Windows shutdown.

https://reviewboard.mozilla.org/r/245832/#review254598

Removing review request for now to wait for the expected update.

::: testing/firefox-ui/harness/firefox_ui_harness/testcases.py:442
(Diff revision 6)
>          Wait(self.marionette, timeout=timeout).until(
>              lambda _: 'applied' in self.software_update.active_update.state,
>              message='Update has been applied.')
> +
> +
> +class TestBaseRestoreWindows(PuppeteerMixin, MarionetteTestCase):

Please don't move that base testcase class into the harness. This is really specific to session restore and should be kept in that folder.
Attachment #8979688 - Flags: review?(hskupin)
Depends on: 1466071
Comment on attachment 8979688 [details]
Bug 1458119: Part 2: Test session restore across Windows shutdown.

https://reviewboard.mozilla.org/r/245832/#review255430

This looks already way better, but lets get the remaining issues sorted out. Please see the inline comments. Thanks.

::: testing/firefox-ui/tests/functional/sessionstore/test_base_restore_windows.py:37
(Diff revision 7)
> +
> +            (self.marionette.absolute_url('layout/mozilla_projects.html'),
> +             self.marionette.absolute_url('layout/mozilla_mission.html')),
> +        ])
> +
> +        self.all_windows = self.test_windows.copy()

Mind moving those 3 lines down to the other if condition in line 55? Lets built-up `all_windows` there.

::: testing/firefox-ui/tests/functional/sessionstore/test_base_restore_windows.py:150
(Diff revision 7)
> +            for tab in win.tabbar.tabs:
> +                urls = urls + tuple([tab.location])
> +            opened_windows.add(urls)
> +        return opened_windows
> +
> +    def os_shutdown(self):

Maybe `simulate_os_shutdown()`?

::: testing/firefox-ui/tests/functional/sessionstore/test_base_restore_windows.py:153
(Diff revision 7)
> +        return opened_windows
> +
> +    def os_shutdown(self):
> +        """ Simulate an OS shutdown, wait until the process exits. """
> +
> +        platformName = self.marionette.session_capabilities['platformName']

The variable is used only once so it's fine to directly use the code where needed.

::: testing/firefox-ui/tests/functional/sessionstore/test_base_restore_windows.py:158
(Diff revision 7)
> +        platformName = self.marionette.session_capabilities['platformName']
> +        pid = self.marionette.process_id
> +
> +        # Perform the shutdown
> +        if platformName == 'windows_nt':
> +            self.assertTrue(TestBaseRestoreWindows.shutdown_with_windows_restart_manager(pid),

Please use `self.shutdown_with_windows_restart_manager`. Maybe then you also don't have to define the extra variable too.

::: testing/firefox-ui/tests/functional/sessionstore/test_base_restore_windows.py:159
(Diff revision 7)
> +        pid = self.marionette.process_id
> +
> +        # Perform the shutdown
> +        if platformName == 'windows_nt':
> +            self.assertTrue(TestBaseRestoreWindows.shutdown_with_windows_restart_manager(pid),
> +                            msg='RmShutdown failed')

I would like to see a better message here. Something like "Restarting process with Windows Restart Manager failed".

Or even better instead of returning a boolean in `shutdown_with_windows_restart_manager` let it raise an exception immediately if something goes wrong. Then there is no need for extra asserts, and it's more Pythonic.

::: testing/firefox-ui/tests/functional/sessionstore/test_base_restore_windows.py:160
(Diff revision 7)
> +
> +        # Perform the shutdown
> +        if platformName == 'windows_nt':
> +            self.assertTrue(TestBaseRestoreWindows.shutdown_with_windows_restart_manager(pid),
> +                            msg='RmShutdown failed')
> +            self.marionette.instance.runner.wait(timeout=self.marionette.DEFAULT_SHUTDOWN_TIMEOUT)

Why is this `wait()` needed? It should all be handled in `marionette.quit()`.

::: testing/firefox-ui/tests/functional/sessionstore/test_base_restore_windows.py:162
(Diff revision 7)
> +        if platformName == 'windows_nt':
> +            self.assertTrue(TestBaseRestoreWindows.shutdown_with_windows_restart_manager(pid),
> +                            msg='RmShutdown failed')
> +            self.marionette.instance.runner.wait(timeout=self.marionette.DEFAULT_SHUTDOWN_TIMEOUT)
> +        else:
> +            self.assertTrue(False, 'Unsupported platform for os_shutdown_restart')

Raise an exception here, and even better negate the if condition, so that we don't need an `else` block.

::: testing/firefox-ui/tests/functional/sessionstore/test_base_restore_windows.py:164
(Diff revision 7)
> +    @staticmethod
> +    def shutdown_with_windows_restart_manager(pid):

I wonder if it would be better to make this a private method and not static. I don't think any test would benefit from both, and we shouldn't call it directly but always through the wrapper method.
Attachment #8979688 - Flags: review?(hskupin) → review-
Comment on attachment 8979688 [details]
Bug 1458119: Part 2: Test session restore across Windows shutdown.

https://reviewboard.mozilla.org/r/245832/#review255430

Continual improvement is the name of the game!

> The variable is used only once so it's fine to directly use the code where needed.

The intent was that it will be used to check against other constants when additional platforms are added, but I'll clean it up for now.

> Why is this `wait()` needed? It should all be handled in `marionette.quit()`.

Good catch, that was left over from before using `marionette.quit()`.
Comment on attachment 8979688 [details]
Bug 1458119: Part 2: Test session restore across Windows shutdown.

https://reviewboard.mozilla.org/r/245832/#review255752

This looks fine to me now. After fixing the remaining issues this is good to get landed. Thanks for adding this nice shutdown method!

::: testing/firefox-ui/tests/functional/sessionstore/test_base_restore_windows.py
(Diff revisions 7 - 8)
>  
>          This function starts a Restart Manager session, registers the
>          process as a resource, and shuts down the process.
> -
> -        @param pid (int)
> -               The process to shutdown

I think it's fine to leave this param in here, and let `simulate_os_shutdown` pass-in the pid. This method here shouldn't know how retrieve the current pid.

::: testing/firefox-ui/tests/functional/sessionstore/test_base_restore_windows.py
(Diff revisions 7 - 8)
> -
> -        @param pid (int)
> -               The process to shutdown
> -
> -        @returns True if the shutdown was completed, False if there
> -                 was some error.

Please use `raises` here, and list the type of exceptions as raised.

Note the correct notation for params and raises is:

:param name: description
:raises: Exception

::: testing/firefox-ui/tests/functional/sessionstore/test_base_restore_windows.py:240
(Diff revisions 7 - 8)
>  
>          # Get the info needed to uniquely identify the process
> +        pid = self.marionette.process_id
>          hProc = OpenProcess(PROCESS_QUERY_INFORMATION, False, pid)
>          if hProc is None:
> -            return False
> +            raise Exception('OpenProcess failed')

Bonus points when you could even add the real failure message via a call to `GetLastError`. It would help a lot when investigating failures.
Attachment #8979688 - Flags: review?(hskupin) → review+
Comment on attachment 8979688 [details]
Bug 1458119: Part 2: Test session restore across Windows shutdown.

https://reviewboard.mozilla.org/r/245832/#review255758

::: testing/firefox-ui/tests/functional/sessionstore/test_base_restore_windows.py:9
(Diff revision 8)
> +
> +from firefox_puppeteer import PuppeteerMixin
> +from marionette_harness import MarionetteTestCase
> +
> +
> +class TestBaseRestoreWindows(PuppeteerMixin, MarionetteTestCase):

Oh, I forgot. Please rename it to `SessionStoreTestCase`.
Comment on attachment 8979688 [details]
Bug 1458119: Part 2: Test session restore across Windows shutdown.

https://reviewboard.mozilla.org/r/245832/#review255758

> Oh, I forgot. Please rename it to `SessionStoreTestCase`.

Can do, renamed the source file as well to reflect this.
Comment on attachment 8979688 [details]
Bug 1458119: Part 2: Test session restore across Windows shutdown.

https://reviewboard.mozilla.org/r/245832/#review255752

Thanks!

> I think it's fine to leave this param in here, and let `simulate_os_shutdown` pass-in the pid. This method here shouldn't know how retrieve the current pid.

Ah ok, I must have misunderstood your earlier suggestion.

> Bonus points when you could even add the real failure message via a call to `GetLastError`. It would help a lot when investigating failures.

I discovered the wonders of `ctypes.WinError()`, which does `GetLastError()` and looks up the human-readable string for the error, returns a `WindowsError`, much more appropriate for this kind of code.
Mike, when you get a chance could you look over SessionStoreTestCase._shutdown_with_windows_restart_manager? Or if you can suggest someone else to review my ctypes shenanigans...
Flags: needinfo?(mdeboer)
Attachment #8979688 - Flags: review?(mdeboer) → review?(gijskruitbosch+bugs)
So I know Gijs has had experience with Win ctypes, so he might/ should be able to assess the sanity of _shutdown_with_windows_restart_manager further - which I think looks good to me.
Flags: needinfo?(mdeboer)
Comment on attachment 8979688 [details]
Bug 1458119: Part 2: Test session restore across Windows shutdown.

https://reviewboard.mozilla.org/r/245832/#review256186

I've only used ctypes from JS, and neither python nor C++ is really my strong suit, but I've listed a bunch of things I noticed which are hopefully helpful...

::: testing/firefox-ui/tests/functional/sessionstore/session_store_test_case.py:1
(Diff revision 9)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.

We normally license test code public domain / CC0.

::: testing/firefox-ui/tests/functional/sessionstore/session_store_test_case.py:189
(Diff revision 9)
> +        OpenProcess = windll.kernel32.OpenProcess
> +        OpenProcess.restype = HANDLE
> +        OpenProcess.argtypes = [DWORD,  # dwDesiredAccess
> +                                BOOL,   # bInheritHandle
> +                                DWORD]  # dwProcessId
> +        PROCESS_QUERY_INFORMATION = DWORD(0x0400)

Curious why you need to cast/construct this as DWORD, but elsewhere compare DWORD result values from ctypes calls against a plain `0` (int?) python type. Seems like they should all be the same - unless python ctypes does conversions for retvals but not to input args, or something?

::: testing/firefox-ui/tests/functional/sessionstore/session_store_test_case.py:214
(Diff revision 9)
> +
> +        RmStartSession = windll.rstrtmgr.RmStartSession
> +        RmStartSession.restype = DWORD
> +        RmStartSession.argtypes = [POINTER(DWORD),  # pSessionHandle
> +                                   DWORD,           # dwSessionFlags
> +                                   POINTER(WCHAR)]  # strSessionKey

Technically this is an array, not a pointer. I don't know if python/ctypes has a way of representing this more semantically? C++ isn't really my strong suit, never mind C++-through-python-ctypes, so maybe it's moot...

::: testing/firefox-ui/tests/functional/sessionstore/session_store_test_case.py:246
(Diff revision 9)
> +        RmEndSession.restype = DWORD
> +        RmEndSession.argtypes = [DWORD]  # dwSessionHandle
> +
> +        # Get the info needed to uniquely identify the process
> +        hProc = OpenProcess(PROCESS_QUERY_INFORMATION, False, pid)
> +        if hProc is None:

I thought idiomatic python was `if not hProc:` (like on line 253) but I could be wrong...

::: testing/firefox-ui/tests/functional/sessionstore/session_store_test_case.py:262
(Diff revision 9)
> +                               pointer(userTime)):
> +            raise WinError()
> +
> +        # Start the Restart Manager Session
> +        dwSessionHandle = DWORD()
> +        sessionKeyType = WCHAR * CCH_RM_SESSION_KEY

Woah. Magic.
Attachment #8979688 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8979688 [details]
Bug 1458119: Part 2: Test session restore across Windows shutdown.

https://reviewboard.mozilla.org/r/245832/#review256186

> We normally license test code public domain / CC0.

Everything I've seen in firefox-ui uses MPL, sticking with that for now.

> Curious why you need to cast/construct this as DWORD, but elsewhere compare DWORD result values from ctypes calls against a plain `0` (int?) python type. Seems like they should all be the same - unless python ctypes does conversions for retvals but not to input args, or something?

Nope, not needed, removed.

> Technically this is an array, not a pointer. I don't know if python/ctypes has a way of representing this more semantically? C++ isn't really my strong suit, never mind C++-through-python-ctypes, so maybe it's moot...

In C at least the formal parameter `WCHAR a[]` is exactly equivalent to `WCHAR *a`, and this header is wrapped in `extern "C" {}`. Thanks for being thorough enough to pick that out!

> I thought idiomatic python was `if not hProc:` (like on line 253) but I could be wrong...

I think you're right, and that's the way I'd write it in C++ anyway.

> Woah. Magic.

Yikes, I'm glad you called that out, I looked it up again and it should be `WCHAR * (CCH_RM_SESSION_KEY + 1)` to reserve space for the nul terminator!
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cc91a5fd7e80
Part 1: Use idleDispatch to be independent from a content window. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/f456143691b9
Part 2: Test session restore across Windows shutdown. r=Gijs,whimboo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cc91a5fd7e80
https://hg.mozilla.org/mozilla-central/rev/f456143691b9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Noticed some slight perf improvments, since comment 58:

== Change summary for alert #14090 (as of Fri, 29 Jun 2018 05:09:41 GMT) ==

Improvements:

  1%  glvideo Mean tick time across 100 ticks:  linux64 opt e10s stylo     14.29 -> 14.09
  1%  glvideo Mean tick time across 100 ticks:  linux64 pgo e10s stylo     14.24 -> 14.09

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14090
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: