1662975 - window.close is not waiting window print dialog

Jorge Silva

Reporter

Description

•

4 years ago

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.

BugBot [:suhaib / :marco/ :calixte]

Comment 1

•

4 years ago

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

Component: Untriaged → Printing: Output

Product: Firefox → Core

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 2

•

4 years ago

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

Flags: needinfo?(jorgefms)

:Gijs (he/him)

Updated

•

4 years ago

Blocks: 133787

Severity: -- → S2

Priority: -- → P2

Whiteboard: [print2020_v81]

:Gijs (he/him)

Comment 4

•

4 years ago

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:

make the printpreview call block (maybe just here) or block with a spin loop or something (ew.)
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.

Flags: needinfo?(jwatt)

Flags: needinfo?(emilio)

:Gijs (he/him)

Updated

•

4 years ago

Status: UNCONFIRMED → NEW

Ever confirmed: true

OS: Unspecified → All

Hardware: Unspecified → Desktop

Jorge Silva

Reporter

Comment 5

•

4 years ago

(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

Flags: needinfo?(jorgefms)

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 6

•

4 years ago

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

Assignee: nobody → emilio

Blake Winton (:bwinton) (:☕️)

Comment 7

•

4 years ago

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

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 8

•

4 years ago

Attached file Bug 1662975 - Don't return from window.print() until the print dialog is closed. r=jwatt,nika — Details

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.

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 9

•

4 years ago

(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

Flags: needinfo?(jwatt)

Flags: needinfo?(emilio)

:Gijs (he/him)

Comment 10

•

4 years ago

(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 )

Pulsebot

Comment 11

•

4 years ago

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

Jonathan Watt [:jwatt]

Comment 12

•

4 years ago

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:

Attachment #9173868 - Flags: approval-mozilla-beta?

Ryan VanderMeulen [:RyanVM][PTO June 24-28]

Comment 13

•

4 years ago

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.

Attachment #9173868 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Ryan VanderMeulen [:RyanVM][PTO June 24-28]

Comment 14

•

4 years ago

bugherder uplift

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

status-firefox81: --- → fixed

Bogdan Tara[:bogdan_tara | bogdant]

Comment 15

•

4 years ago

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

Flags: needinfo?(emilio)

Hiroyuki Ikezoe (:hiro)

Comment 16

•

4 years ago

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.

Flags: needinfo?(btara)

Pulsebot

Comment 17

•

4 years ago

Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/69dba5223556
Don't return from window.print() until the print dialog is closed. r=jwatt

Bogdan Tara[:bogdan_tara | bogdant]

Comment 18

•

4 years ago

Yes. Sorry. Relanded.

Flags: needinfo?(emilio)

Flags: needinfo?(btara)

Hiroyuki Ikezoe (:hiro)

Comment 19

•

4 years ago

No worries! Thanks!

Hiroyuki Ikezoe (:hiro)

Updated

•

4 years ago

Status: NEW → ASSIGNED

Bogdan Tara[:bogdan_tara | bogdant]

Comment 20

•

4 years ago

bugherder

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

Status: ASSIGNED → RESOLVED

Closed: 4 years ago

status-firefox82: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → 82 Branch

Camelia Badau [:cbadau], Desktop QA

Updated

•

4 years ago

Flags: qe-verify+

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

Comment 21

•

4 years ago

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.

Flags: needinfo?(emilio)

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 22

•

4 years ago

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.

Brindusa Tot, Desktop QA

Updated

•

4 years ago

QA Whiteboard: [qa-triaged]

Alice0775 White

Updated

•

4 years ago

Regressions: 1663140

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 23

•

4 years ago

Added one in bug 1663140.

Flags: needinfo?(emilio)

Emil Ghitta, QA [:emilghitta]

Comment 24

•

4 years ago

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.

Status: RESOLVED → VERIFIED

status-firefox81: fixed → verified

status-firefox82: fixed → verified

Flags: qe-verify+

:Gijs (he/him)

Updated

•

4 years ago

Comment 26

•

4 years ago

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

ElDiablo

Comment 27

•

4 years ago

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>

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 28

•

4 years ago

(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.

Emilio Cobos Álvarez (:emilio)

Assignee

Updated

•

4 years ago

Blocks: 1667350

Jonathan Watt [:jwatt]

Updated

•

3 years ago