1662975 - window.close is not waiting window print dialog

Status: VERIFIED FIXED

(Core :: Printing: Output, defect, P2)

Description

[Jorge Silva]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:81.0) Gecko/20100101 Firefox/81.0

Steps to reproduce:

newWin=window.open();
newWin.focus();
newWin.print();
newWin.close();

Actual results:

The page closes without waiting the print dialog box.

Expected results:

The page should closes only after click on print dialog box to print or cancel, like in all the old print preview.
With print.tab_modal.enabled set to False it works ok.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Can you attach a test-case to the bug? I think this should be fixed on Nightly via bug 1636728, but want to confirm.

Comment 4

[:Gijs (out for now; he/him)]

OK, so I can repro by just running those 4 lines from comment #0 in a web console for e.g. this bugzilla page.

The difference between the broken and working case is that in nsGlobalWindowOuter::PrintOuter at https://searchfox.org/mozilla-central/rev/2b250967a66886398e5e798371484fd018d88a22/dom/base/nsGlobalWindowOuter.cpp#5268-5269 , we determine that we're going to "print preview", and so in Print on the same class, for this code:

  if (aIsPreview) {
    aError = webBrowserPrint->PrintPreview(aPrintSettings, aListener,
                                           std::move(aPrintPreviewCallback));
  } else {
    // Historically we've eaten this error.
    webBrowserPrint->Print(aPrintSettings, aListener);
  }

we take the aIsPreview == true path, which doesn't block.

So obvious options I see include:

1. make the printpreview call block (maybe just here) or block with a spin loop or something (ew.)
2. prevent website-initiated window closure if we hit this path. It looks like nsDocumentViewer::RequestWindowClose already has some logic to check for a pending print job, and defer the close until the print job is finished if so, which presumably we could build on?

So I suspect we should go for 2 - Emilio / :jwatt, any gut sense of how best to orchestrate that? We should already be able to determine whether the window is in modal state, but I doubt we want to block window closing at all times for that reason (e.g. would want users to be able to close the tab!). I don't know this code well enough to know if there's already something more specific exposed on the window/docshell/browsingcontext/contentviewer that we can use, or if we need to introduce a new bool or something.

Comment 5

[Jorge Silva]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

Can you attach a test-case to the bug? I think this should be fixed on Nightly via bug 1636728, but want to confirm.

Just run these 4 lines in console:
newWin=window.open();
newWin.focus();
newWin.print();
newWin.close();

Test with print.tab_modal.enabled TRUE and FALSE to see the difference

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Yeah, we probably need to make window.print() block.

Comment 7

[Blake Winton (:bwinton) (:☕️)]

As a note, window.print() isn't specified as blocking or not, and Safari has it non-blocking…

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1662975 - Don't return from window.print() until the print dialog is closed. r=jwatt,nika

As much as I hate nested event loops, I think we need to do it for this
case :)

Hopefully print() already assumes nested event loops can happen because
of the native dialog so it's already properly set up for this.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #7)

As a note, window.print() isn't specified as blocking or not, and Safari has it non-blocking…

So safari behaves like us on the test-case above? I don't recall that behavior when testing window.print on WebKit... But I can double-check I guess

Comment 10

[:Gijs (out for now; he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #9)

(In reply to Blake Winton (:bwinton) (:☕️) from comment #7)

As a note, window.print() isn't specified as blocking or not, and Safari has it non-blocking…

So safari behaves like us on the test-case above? I don't recall that behavior when testing window.print on WebKit... But I can double-check I guess

It blocks in Safari Version 13.1.2 (15609.3.5.1.3) on macOS, despite what MDN says ( https://developer.mozilla.org/en-US/docs/Web/API/Window/print )

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/52d0f9a901a9
Don't return from window.print() until the print dialog is closed. r=jwatt

Comment 12

[Jonathan Watt [:jwatt]]

Comment on attachment 9173868 [details]
Bug 1662975 - Don't return from window.print() until the print dialog is closed. r=jwatt,nika

Beta/Release Uplift Approval Request

• User impact if declined: Site initiated ("Print" button) printing can be broken.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed:

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9173868 [details]
Bug 1662975 - Don't return from window.print() until the print dialog is closed. r=jwatt,nika

Approved for 81.0b6. Fingers crossed I guess.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b52d12a944c1

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset 52d0f9a901a9 (bug 1662975) for nested_fullscreen.https.html failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=os%2Cx%2C10.14%2Copt%2Cweb%2Cplatform%2Ctests%2Ctest-macosx1014-64%2Fopt-web-platform-tests-e10s%2Cwpt5&fromchange=008d2bdf5bd5b15193de8d9b7c0df8b23c6ecb4e&tochange=aabe6661eb34db4590e7159dd3f3825d26c071db&selectedTaskRun=bKsJx-lUQLeSZKZh5z0UYA.0

Backout link: https://hg.mozilla.org/integration/autoland/rev/aabe6661eb34db4590e7159dd3f3825d26c071db

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=314826366&repo=autoland&lineNumber=7969

[task 2020-09-03T20:34:26.308Z] 20:34:26     INFO - TEST-START | /webxr/dom-overlay/nested_fullscreen.https.html
[task 2020-09-03T20:34:26.308Z] 20:34:26     INFO - Closing window 40
[task 2020-09-03T20:34:51.323Z] 20:34:51     INFO - Got timeout in harness
[task 2020-09-03T20:34:51.323Z] 20:34:51     INFO - TEST-UNEXPECTED-TIMEOUT | /webxr/dom-overlay/nested_fullscreen.https.html | TestRunner hit external timeout (this may indicate a hang)
[task 2020-09-03T20:34:51.323Z] 20:34:51     INFO - TEST-INFO took 25038ms
[task 2020-09-03T20:34:51.333Z] 20:34:51     INFO - Restarting browser for new test group
[task 2020-09-03T20:36:05.729Z] 20:36:05     INFO - Browser exited with return code -15
[task 2020-09-03T20:36:05.729Z] 20:36:05     INFO - PROCESS LEAKS None
[task 2020-09-03T20:36:05.729Z] 20:36:05     INFO - Browser not responding, setting status to CRASH
[task 2020-09-03T20:36:05.729Z] 20:36:05  WARNING - Command left in command_queue during cleanup: 'test_ended', (<wptrunner.wpttest.TestharnessTest /webxr/dom-overlay/nested_fullscreen.https.html>, (<wptrunner.wpttest.TestharnessResult CRASH>, []))
[task 2020-09-03T20:36:05.729Z] 20:36:05     INFO - Closing logging queue
[task 2020-09-03T20:36:05.729Z] 20:36:05  WARNING - Action click failed
[task 2020-09-03T20:36:05.729Z] 20:36:05  WARNING - Traceback (most recent call last):
[task 2020-09-03T20:36:05.730Z] 20:36:05  WARNING -   File "/Users/cltbld/tasks/task_1599163735/build/tests/web-platform/tests/tools/wptrunner/wptrunner/executors/base.py", line 790, in process_action
[task 2020-09-03T20:36:05.730Z] 20:36:05  WARNING -     result = action_handler(payload)
[task 2020-09-03T20:36:05.730Z] 20:36:05  WARNING -   File "/Users/cltbld/tasks/task_1599163735/build/tests/web-platform/tests/tools/wptrunner/wptrunner/executors/actions.py", line 13, in __call__
[task 2020-09-03T20:36:05.730Z] 20:36:05  WARNING -     self.protocol.click.element(element)
[task 2020-09-03T20:36:05.730Z] 20:36:05  WARNING -   File "/Users/cltbld/tasks/task_1599163735/build/tests/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py", line 441, in element
[task 2020-09-03T20:36:05.730Z] 20:36:05  WARNING -     return element.click()
[task 2020-09-03T20:36:05.730Z] 20:36:05  WARNING -   File "/Users/cltbld/tasks/task_1599163735/build/venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 257, in click
[task 2020-09-03T20:36:05.730Z] 20:36:05  WARNING -     {"id": self.id})
[task 2020-09-03T20:36:05.730Z] 20:36:05  WARNING -   File "/Users/cltbld/tasks/task_1599163735/build/venv/lib/python2.7/site-packages/marionette_driver/decorators.py", line 36, in _
[task 2020-09-03T20:36:05.730Z] 20:36:05  WARNING -     m._handle_socket_failure()
[task 2020-09-03T20:36:05.730Z] 20:36:05  WARNING -   File "/Users/cltbld/tasks/task_1599163735/build/venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 654, in _handle_socket_failure
[task 2020-09-03T20:36:05.730Z] 20:36:05  WARNING -     reraise(exc_cls, exc, tb)
[task 2020-09-03T20:36:05.730Z] 20:36:05  WARNING -   File "/Users/cltbld/tasks/task_1599163735/build/venv/lib/python2.7/site-packages/marionette_driver/decorators.py", line 26, in _
[task 2020-09-03T20:36:05.730Z] 20:36:05  WARNING -     return func(*args, **kwargs)
[task 2020-09-03T20:36:05.730Z] 20:36:05  WARNING -   File "/Users/cltbld/tasks/task_1599163735/build/venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 594, in _send_message
[task 2020-09-03T20:36:05.731Z] 20:36:05  WARNING -     msg = self.client.request(name, params)
[task 2020-09-03T20:36:05.731Z] 20:36:05  WARNING -   File "/Users/cltbld/tasks/task_1599163735/build/venv/lib/python2.7/site-packages/marionette_driver/transport.py", line 276, in request
[task 2020-09-03T20:36:05.731Z] 20:36:05  WARNING -     return self.receive()
[task 2020-09-03T20:36:05.731Z] 20:36:05  WARNING -   File "/Users/cltbld/tasks/task_1599163735/build/venv/lib/python2.7/site-packages/marionette_driver/transport.py", line 162, in receive
[task 2020-09-03T20:36:05.731Z] 20:36:05  WARNING -     raise socket.error("No data received over socket")
[task 2020-09-03T20:36:05.731Z] 20:36:05  WARNING - error: No data received over socket
[task 2020-09-03T20:36:05.731Z] 20:36:05  WARNING - 
[task 2020-09-03T20:36:05.731Z] 20:36:05  WARNING - Traceback (most recent call last):
[task 2020-09-03T20:36:05.731Z] 20:36:05  WARNING -   File "/Users/cltbld/tasks/task_1599163735/build/tests/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py", line 729, in run_func
[task 2020-09-03T20:36:05.731Z] 20:36:05  WARNING -     self.result = True, self.func(self.protocol, self.url, self.timeout)
[task 2020-09-03T20:36:05.731Z] 20:36:05  WARNING -   File "/Users/cltbld/tasks/task_1599163735/build/tests/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py", line 846, in do_testharness
[task 2020-09-03T20:36:05.731Z] 20:36:05  WARNING -     done, rv = handler(result)
[task 2020-09-03T20:36:05.731Z] 20:36:05  WARNING -   File "/Users/cltbld/tasks/task_1599163735/build/tests/web-platform/tests/tools/wptrunner/wptrunner/executors/base.py", line 776, in __call__
[task 2020-09-03T20:36:05.731Z] 20:36:05  WARNING -     return callback(url, payload)
[task 2020-09-03T20:36:05.731Z] 20:36:05  WARNING -   File "/Users/cltbld/tasks/task_1599163735/build/tests/web-platform/tests/tools/wptrunner/wptrunner/executors/base.py", line 797, in process_action
[task 2020-09-03T20:36:05.731Z] 20:36:05  WARNING -     self._send_message("complete", "error")
[task 2020-09-03T20:36:05.731Z] 20:36:05  WARNING -   File "/Users/cltbld/tasks/task_1599163735/build/tests/web-platform/tests/tools/wptrunner/wptrunner/executors/base.py", line 807, in _send_message
[task 2020-09-03T20:36:05.731Z] 20:36:05  WARNING -     self.protocol.testdriver.send_message(message_type, status, message=message)
[task 2020-09-03T20:36:05.731Z] 20:36:05  WARNING -   File "/Users/cltbld/tasks/task_1599163735/build/tests/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py", line 473, in send_message
[task 2020-09-03T20:36:05.732Z] 20:36:05  WARNING -     self.parent.base.execute_script("window.postMessage(%s, '*')" % json.dumps(obj))
[task 2020-09-03T20:36:05.732Z] 20:36:05  WARNING -   File "/Users/cltbld/tasks/task_1599163735/build/tests/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py", line 87, in execute_script
[task 2020-09-03T20:36:05.732Z] 20:36:05  WARNING -     return method(script, new_sandbox=False, sandbox=None)
[task 2020-09-03T20:36:05.732Z] 20:36:05  WARNING -   File "/Users/cltbld/tasks/task_1599163735/build/venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 1592, in execute_script
[task 2020-09-03T20:36:05.732Z] 20:36:05  WARNING -     rv = self._send_message("WebDriver:ExecuteScript", body, key="value")
[task 2020-09-03T20:36:05.732Z] 20:36:05  WARNING -   File "/Users/cltbld/tasks/task_1599163735/build/venv/lib/python2.7/site-packages/marionette_driver/decorators.py", line 26, in _
[task 2020-09-03T20:36:05.732Z] 20:36:05  WARNING -     return func(*args, **kwargs)
[task 2020-09-03T20:36:05.732Z] 20:36:05  WARNING -   File "/Users/cltbld/tasks/task_1599163735/build/venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 591, in _send_message
[task 2020-09-03T20:36:05.732Z] 20:36:05  WARNING -     raise errors.InvalidSessionIdException("Please start a session")
[task 2020-09-03T20:36:05.732Z] 20:36:05  WARNING - InvalidSessionIdException: Please start a session
[task 2020-09-03T20:36:05.732Z] 20:36:05  WARNING - 
[task 2020-09-03T20:36:05.732Z] 20:36:05     INFO - queue closed
[task 2020-09-03T20:36:05.758Z] 20:36:05     INFO - Application command: /Users/cltbld/tasks/task_1599163735/build/application/Firefox Nightly.app/Contents/MacOS/firefox --marionette about:blank -foreground -profile /var/folders/82/wd5jdtj95kq8kjnyxp1wjrhc000017/T/tmpHGX9sw
[task 2020-09-03T20:36:05.763Z] 20:36:05     INFO - Starting runner
[task 2020-09-03T20:36:08.178Z] 20:36:08     INFO - PID 1853 | console.error: SearchCache: "_readCacheFile: Error reading cache file:" (new Error("", "(unknown module)"))
[task 2020-09-03T20:36:09.399Z] 20:36:09     INFO - PID 1853 | 1599165369377	Marionette	INFO	Listening on port 64713
[task 2020-09-03T20:36:09.632Z] 20:36:09     INFO - TEST-START | /WebIDL/ecmascript-binding/legacy-platform-object/DefineOwnProperty.html

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Bogdan, it looks like https://hg.mozilla.org/integration/autoland/rev/4519f0bb8624c6f1d4ee1c7a971a973d51f8193a which landed just after this bug's commit is the culprit of the failure.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69dba5223556
Don't return from window.print() until the print dialog is closed. r=jwatt

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

Yes. Sorry. Relanded.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

No worries! Thanks!

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/69dba5223556

Comment 21

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

Aren't we creating any browser chrome tests for the new print feature? Given the importance of this bug I would have expected to see at least one landed together with the patch here.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Yes, there are. Our window.print() tests are pretty scarce though, and I was on a rush writing this yesterday (I was technically on PTO...). I'll write one though, leaving ni? around.

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Added one in bug 1663140.

Comment 24

[Emil Ghitta, QA [:emilghitta]]

Reproduced this issue using Firefox 82.0a1 (BuildId:20200903151816).

This issue is verified fixed using Firefox 82.0a1 (BuildId:20200906215057) and Firefox 81.0b7 (BuildId:20200906164749) on Windows 10 64bit, macOS 10.14 & Ubuntu 18.04 64bit.

Comment 26

[ElDiablo]

We are still having this bug with Firefox 82.0b3 and 83.0a1 (2020-09-24)

Comment 27

[ElDiablo]

Here is a small test case we used on success with Firefox 80, but not with 81 and further builds :

<!DOCTYPE html>
<html>
<body>
Hello, World !
<script type="text/javascript">
window.print();
console.log('Test');
window.close();
</script>
</html>

Comment 28

[Emilio Cobos Álvarez (:emilio)]

(In reply to ElDiablo from comment #27)
Ah, so that's different. That's because you're calling window.print() before onload fires, so we delay it, and we should be delaying window.close() too, probably, instead of forgetting about it.

Your test-case does if you do something like this:

<!doctype html>
<html>
<body>
Hello, World !
<script type="text/javascript">
onload = function() {
  setTimeout(function() {
    window.print();
    console.log('Test');
    window.close();
  }, 0);
};
</script>
</html>

Will file a separate bug for this.