1668586 - Print preview doesn’t get closed if navigating away from a local (about page) to a remote one and the tab in question becomes inaccessible

Emil Ghitta, QA [:emilghitta]

Reporter

Description

•

4 years ago

Attached image aboutToRemote.gif (deleted) — Details

Affected versions

83.0a1 (BuildId:20200930214529)
82.0b5 (BuildId:20200929175845)

Affected platforms

Windows 10 64bit
Ubuntu 18.04 64bit
macOS 10.14

Steps to reproduce

Launch Firefox.
Access the about:support page.
Hit Ctrl + P.
Type something in the address bar and hit enter to navigate.

Expected result

Navigation can be successfully performed and the print preview closes.

Actual result

Navigation can be successfully performed but the print preview remains open. Also the tab becomes inaccessible.

Regression Window

I will search for the regressor once time permits.

Additional Information

[Suggested Severity] S2

Notes
This doesn’t seem reproducible while navigating from:

Remote - Remote
Remote - Local
Local -Local

:Gijs (he/him)

Assignee

Comment 1

•

4 years ago

This looks like an issue with prompts; running Services.prompt.alertBC(gBrowser.selectedBrowser.browsingContext, Ci.nsIPrompt.MODAL_TYPE_TAB, "hi", "hi") instead of opening the print dialog has the same result.

Component: Printing → Notifications and Alerts

Flags: needinfo?(gijskruitbosch+bugs)

Blake Winton (:bwinton) (:☕️)

Comment 2

•

4 years ago

In that case, I'll remove it from the print2020_v82 list… :)

Whiteboard: [print2020_v82] [old-ui-]

Paul Zühlcke [:pbz]

Comment 3

•

4 years ago

There seems to be a problem with restoring progress listeners when updating the browser from local to remote.
The addProgressListener call fails: https://searchfox.org/mozilla-central/rev/222e4f64b769413ac1a1991d2397b13a0acb5d9d/toolkit/content/widgets/browser-custom-element.js#909,934,1959
These calls come from finishChangeRemoteness

The listener is initially registered here (when a dialog / prompt opens): https://searchfox.org/mozilla-central/rev/222e4f64b769413ac1a1991d2397b13a0acb5d9d/browser/base/content/browser.js#9024

:Gijs (he/him)

Assignee

Comment 4

•

4 years ago

(In reply to Paul Zühlcke [:pbz] from comment #3)

There seems to be a problem with restoring progress listeners when updating the browser from local to remote.
The addProgressListener call fails: https://searchfox.org/mozilla-central/rev/222e4f64b769413ac1a1991d2397b13a0acb5d9d/toolkit/content/widgets/browser-custom-element.js#909,934,1959
These calls come from finishChangeRemoteness

The listener is initially registered here (when a dialog / prompt opens): https://searchfox.org/mozilla-central/rev/222e4f64b769413ac1a1991d2397b13a0acb5d9d/browser/base/content/browser.js#9024

Paul is correct. The failure of addProgressListener is because the listener is already listed for this browser.

Now, of course we could wallpaper over that (ie catch that error and ignore it in the browser-custom-element.js callsite). But the thing that confuses me is that we call destroy() and then construct() when switching remoteness, but the former does not remove progress listeners, but the latter does add them. So I'm at the "how did this ever work" stage of trying to understand what's happening here - why aren't we seeing this error for every progress listener ever attached to a browser that remoteness switches? Does this only work for tabbed browsers because the tabbrowser destroys and re-adds its own listener at https://searchfox.org/mozilla-central/rev/7ef5cefd0468b8f509efe38e0212de2398f4c8b3/browser/base/content/tabbrowser.js#1817-1821 ? Matt, do you know?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(matt.woodrow)

Matt Woodrow (:mattwoodrow)

Comment 5

•

4 years ago

I think you're correct that it's broken, but tabbrowser works around it by accident.

We used to add progress listeners to the docshell directly when we were a local <browser>, and to a RemoteWebProgress object for remote ones. That would mean that we'd lose all the old listeners when changing between those two states and wouldn't need to explicitly remove them.

Now that we're always using the same BrowsingContextWebProgress, we shouldn't need to re-add listeners when we change remoteness.

Flags: needinfo?(matt.woodrow)

Camelia Badau [:cbadau], Desktop QA

Updated

•

4 years ago

Has Regression Range: --- → no

Has STR: --- → yes

:Gijs (he/him)

Assignee

Updated

•

4 years ago

Assignee: nobody → gijskruitbosch+bugs

:Gijs (he/him)

Assignee

Comment 6

•

4 years ago

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

:Gijs (he/him)

Assignee

Comment 7

•

4 years ago

Attached file Bug 1668586 - stop messing with web progress listeners when changing remoteness, r?mattwoodrow!,agi! (deleted) — Details

:Gijs (he/him)

Assignee

Comment 8

•

4 years ago

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)

Now that we're always using the same BrowsingContextWebProgress, we shouldn't need to re-add listeners when we change remoteness.

Turns out we can just remove some code and try is green, so yay?

Pulsebot

Comment 9

•

4 years ago

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/dca68a64b04c stop messing with web progress listeners when changing remoteness, r=mattwoodrow

Bogdan Tara[:bogdan_tara | bogdant]

Comment 10

•

4 years ago

Backed out changeset dca68a64b04c (bug 1668586) for test_ext_tabs failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=android%2Copt%2Cmochitest&tochange=19bc84a9ed83efac55c260d5437c3426eeb36b49&fromchange=0fe2a7dc580cc1a3a0ea6f607f2e4e92b2f1be37&selectedTaskRun=JuqSO2sNQxChaxGJM5EfuA.0

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

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

[task 2020-10-06T10:28:28.370Z] 10:28:28     INFO -  3628 INFO TEST-START | mobile/android/components/extensions/test/mochitest/test_ext_tabs_create.html
[task 2020-10-06T10:33:55.272Z] 10:33:55     INFO -  Buffered messages logged at 10:28:17
[task 2020-10-06T10:33:55.272Z] 10:33:55     INFO -  3629 INFO add_task | Entering test
[task 2020-10-06T10:33:55.272Z] 10:33:55     INFO -  3630 INFO Extension loaded
[task 2020-10-06T10:33:55.272Z] 10:33:55     INFO -  Buffered messages logged at 10:28:18
[task 2020-10-06T10:33:55.272Z] 10:33:55     INFO -  3631 INFO Testing tabs.create({"url":"http://example.com/"}), expecting {"url":"http://example.com/"}
[task 2020-10-06T10:33:55.273Z] 10:33:55     INFO -  3632 INFO TEST-PASS | mobile/android/components/extensions/test/mochitest/test_ext_tabs_create.html | Expected tabs.onCreated callback to receive tab object
[task 2020-10-06T10:33:55.273Z] 10:33:55     INFO -  3633 INFO TEST-PASS | mobile/android/components/extensions/test/mochitest/test_ext_tabs_create.html | Expected value for tab.active - Expected: true, Actual: true
[task 2020-10-06T10:33:55.273Z] 10:33:55     INFO -  3634 INFO TEST-PASS | mobile/android/components/extensions/test/mochitest/test_ext_tabs_create.html | Expected value for tab.id - Expected: 10017, Actual: 10017
[task 2020-10-06T10:33:55.273Z] 10:33:55     INFO -  3635 INFO TEST-PASS | mobile/android/components/extensions/test/mochitest/test_ext_tabs_create.html | Expected value for tab.url - Expected: http://example.com/, Actual: http://example.com/
[task 2020-10-06T10:33:55.274Z] 10:33:55     INFO -  3636 INFO Testing tabs.create({"url":"blank.html"}), expecting {"url":"moz-extension://abd955e2-18ac-4d2b-a29e-713bceec2acc/bg/blank.html"}
[task 2020-10-06T10:33:55.274Z] 10:33:55     INFO -  3637 INFO TEST-PASS | mobile/android/components/extensions/test/mochitest/test_ext_tabs_create.html | Expected tabs.onCreated callback to receive tab object
[task 2020-10-06T10:33:55.274Z] 10:33:55     INFO -  3638 INFO TEST-PASS | mobile/android/components/extensions/test/mochitest/test_ext_tabs_create.html | Expected tabs.onCreated callback to receive tab object
[task 2020-10-06T10:33:55.274Z] 10:33:55     INFO -  3639 INFO TEST-PASS | mobile/android/components/extensions/test/mochitest/test_ext_tabs_create.html | Expected value for tab.active - Expected: true, Actual: true
[task 2020-10-06T10:33:55.274Z] 10:33:55     INFO -  Buffered messages finished
[task 2020-10-06T10:33:55.275Z] 10:33:55  WARNING -  3640 INFO TEST-UNEXPECTED-FAIL | mobile/android/components/extensions/test/mochitest/test_ext_tabs_create.html | Test timed out.
[task 2020-10-06T10:33:55.275Z] 10:33:55     INFO -      SimpleTest.ok@SimpleTest/SimpleTest.js:417:16
[task 2020-10-06T10:33:55.275Z] 10:33:55     INFO -      reportError@SimpleTest/TestRunner.js:143:22
[task 2020-10-06T10:33:55.275Z] 10:33:55     INFO -      TestRunner._checkForHangs@SimpleTest/TestRunner.js:165:18
[task 2020-10-06T10:33:55.275Z] 10:33:55  WARNING -  3641 INFO TEST-UNEXPECTED-FAIL | mobile/android/components/extensions/test/mochitest/test_ext_tabs_create.html | Extension left running at test shutdown
[task 2020-10-06T10:33:55.275Z] 10:33:55     INFO -      SimpleTest.ok@SimpleTest/SimpleTest.js:417:16
[task 2020-10-06T10:33:55.275Z] 10:33:55     INFO -      ExtensionTestUtils.loadExtension/<@SimpleTest/ExtensionTestUtils.js:117:18
[task 2020-10-06T10:33:55.275Z] 10:33:55     INFO -      executeCleanupFunction@SimpleTest/SimpleTest.js:1614:13
[task 2020-10-06T10:33:55.275Z] 10:33:55     INFO -      SimpleTest.finish@SimpleTest/SimpleTest.js:1628:3
[task 2020-10-06T10:33:55.275Z] 10:33:55     INFO -      killTest@SimpleTest/TestRunner.js:152:22
[task 2020-10-06T10:33:55.276Z] 10:33:55     INFO -  3642 INFO TEST-OK | mobile/android/components/extensions/test/mochitest/test_ext_tabs_create.html | took 329611ms
[task 2020-10-06T10:33:55.276Z] 10:33:55  WARNING -  3643 ERROR TEST-UNEXPECTED-FAIL | /tests/mobile/android/components/extensions/test/mochitest/test_ext_tabs_create.html logged result after SimpleTest.finish(): Extension left running at test shutdown
[task 2020-10-06T10:33:55.276Z] 10:33:55     INFO -  3644 INFO TEST-START | mobile/android/components/extensions/test/mochitest/test_ext_tabs_events.html
[task 2020-10-06T10:33:55.276Z] 10:33:55     INFO -  3645 INFO TEST-OK | mobile/android/components/extensions/test/mochitest/test_ext_tabs_events.html | took 336ms
[task 2020-10-06T10:33:55.277Z] 10:33:55     INFO -  3646 INFO TEST-START | mobile/android/components/extensions/test/mochitest/test_ext_tabs_executeScript.html
[task 2020-10-06T10:33:55.278Z] 10:33:55     INFO -  3647 INFO TEST-OK | mobile/android/components/extensions/test/mochitest/test_ext_tabs_executeScript.html | took 652ms
[task 2020-10-06T10:33:55.278Z] 10:33:55     INFO -  3648 INFO TEST-START | mobile/android/components/extensions/test/mochitest/test_ext_tabs_executeScript_bad.html
[task 2020-10-06T10:33:55.279Z] 10:33:55     INFO -  3649 INFO TEST-OK | mobile/android/components/extensions/test/mochitest/test_ext_tabs_executeScript_bad.html | took 190ms
[task 2020-10-06T10:33:55.279Z] 10:33:55     INFO -  3650 INFO TEST-START | mobile/android/components/extensions/test/mochitest/test_ext_tabs_executeScript_good.html
[task 2020-10-06T10:33:55.280Z] 10:33:55     INFO -  3651 INFO TEST-OK | mobile/android/components/extensions/test/mochitest/test_ext_tabs_executeScript_good.html | took 329ms
[task 2020-10-06T10:33:55.280Z] 10:33:55     INFO -  3652 INFO TEST-START | mobile/android/components/extensions/test/mochitest/test_ext_tabs_executeScript_no_create.html
[task 2020-10-06T10:33:55.281Z] 10:33:55     INFO -  3653 INFO TEST-OK | mobile/android/components/extensions/test/mochitest/test_ext_tabs_executeScript_no_create.html | took 266ms
[task 2020-10-06T10:33:55.281Z] 10:33:55     INFO -  3654 INFO TEST-START | mobile/android/components/extensions/test/mochitest/test_ext_tabs_executeScript_runAt.html
[task 2020-10-06T10:33:55.281Z] 10:33:55     INFO -  3655 INFO TEST-OK | mobile/android/components/extensions/test/mochitest/test_ext_tabs_executeScript_runAt.html | took 4264ms
[task 2020-10-06T10:33:55.281Z] 10:33:55     INFO -  3656 INFO TEST-START | mobile/android/components/extensions/test/mochitest/test_ext_tabs_get.html
[task 2020-10-06T10:34:05.724Z] 10:34:05     INFO -  3657 INFO TEST-OK | mobile/android/components/extensions/test/mochitest/test_ext_tabs_get.html | took 217ms
[task 2020-10-06T10:34:05.725Z] 10:34:05     INFO -  3658 INFO TEST-START | mobile/android/components/extensions/test/mochitest/test_ext_tabs_getCurrent.html
[task 2020-10-06T10:34:05.725Z] 10:34:05     INFO -  3659 INFO TEST-OK | mobile/android/components/extensions/test/mochitest/test_ext_tabs_getCurrent.html | took 212ms
[task 2020-10-06T10:34:05.726Z] 10:34:05     INFO -  3660 INFO TEST-START | mobile/android/components/extensions/test/mochitest/test_ext_tabs_goBack_goForward.html
[task 2020-10-06T10:39:22.041Z] 10:39:22     INFO -  Buffered messages logged at 10:33:55
[task 2020-10-06T10:39:22.041Z] 10:39:22     INFO -  3661 INFO add_task | Entering test test_tabs_goBack_goForward
[task 2020-10-06T10:39:22.041Z] 10:39:22     INFO -  3662 INFO Extension loaded
[task 2020-10-06T10:39:22.041Z] 10:39:22     INFO -  Buffered messages finished
[task 2020-10-06T10:39:22.042Z] 10:39:22  WARNING -  3663 INFO TEST-UNEXPECTED-FAIL | mobile/android/components/extensions/test/mochitest/test_ext_tabs_goBack_goForward.html | Test timed out.
[task 2020-10-06T10:39:22.042Z] 10:39:22     INFO -      SimpleTest.ok@SimpleTest/SimpleTest.js:417:16
[task 2020-10-06T10:39:22.042Z] 10:39:22     INFO -      reportError@SimpleTest/TestRunner.js:143:22
[task 2020-10-06T10:39:22.042Z] 10:39:22     INFO -      TestRunner._checkForHangs@SimpleTest/TestRunner.js:165:18
[task 2020-10-06T10:39:22.042Z] 10:39:22  WARNING -  3664 INFO TEST-UNEXPECTED-FAIL | mobile/android/components/extensions/test/mochitest/test_ext_tabs_goBack_goForward.html | Extension left running at test shutdown
[task 2020-10-06T10:39:22.043Z] 10:39:22     INFO -      SimpleTest.ok@SimpleTest/SimpleTest.js:417:16
[task 2020-10-06T10:39:22.043Z] 10:39:22     INFO -      ExtensionTestUtils.loadExtension/<@SimpleTest/ExtensionTestUtils.js:117:18
[task 2020-10-06T10:39:22.043Z] 10:39:22     INFO -      executeCleanupFunction@SimpleTest/SimpleTest.js:1614:13
[task 2020-10-06T10:39:22.043Z] 10:39:22     INFO -      SimpleTest.finish@SimpleTest/SimpleTest.js:1628:3
[task 2020-10-06T10:39:22.043Z] 10:39:22     INFO -      killTest@SimpleTest/TestRunner.js:152:22
[task 2020-10-06T10:39:22.043Z] 10:39:22     INFO -  3665 INFO TEST-OK | mobile/android/components/extensions/test/mochitest/test_ext_tabs_goBack_goForward.html | took 322322ms
[task 2020-10-06T10:39:22.043Z] 10:39:22  WARNING -  3666 ERROR TEST-UNEXPECTED-FAIL | /tests/mobile/android/components/extensions/test/mochitest/test_ext_tabs_goBack_goForward.html logged result after SimpleTest.finish(): Extension left running at test shutdown
[task 2020-10-06T10:39:22.044Z] 10:39:22     INFO -  3667 INFO TEST-START | mobile/android/components/extensions/test/mochitest/test_ext_tabs_insertCSS.html
[task 2020-10-06T10:39:22.045Z] 10:39:22     INFO -  3668 INFO TEST-OK | mobile/android/components/extensions/test/mochitest/test_ext_tabs_insertCSS.html | took 288ms
[task 2020-10-06T10:39:22.045Z] 10:39:22     INFO -  3669 INFO TEST-START | mobile/android/components/extensions/test/mochitest/test_ext_tabs_onUpdated.html
[task 2020-10-06T10:39:22.046Z] 10:39:22     INFO -  3670 INFO TEST-OK | mobile/android/components/extensions/test/mochitest/test_ext_tabs_onUpdated.html | took 263ms
[task 2020-10-06T10:39:22.046Z] 10:39:22     INFO -  3671 INFO TEST-START | mobile/android/components/extensions/test/mochitest/test_ext_tabs_query.html
[task 2020-10-06T10:39:22.047Z] 10:39:22     INFO -  3672 INFO TEST-OK | mobile/android/components/extensions/test/mochitest/test_ext_tabs_query.html | took 194ms
[task 2020-10-06T10:39:22.047Z] 10:39:22     INFO -  3673 INFO TEST-START | mobile/android/components/extensions/test/mochitest/test_ext_tabs_sendMessage.html
[task 2020-10-06T10:39:22.050Z] 10:39:22     INFO -  3674 INFO TEST-OK | mobile/android/components/extensions/test/mochitest/test_ext_tabs_sendMessage.html | took 563ms
[task 2020-10-06T10:39:22.050Z] 10:39:22     INFO -  3675 INFO TEST-START | mobile/android/components/extensions/test/mochitest/test_ext_tabs_update_url.html
[task 2020-10-06T10:39:32.493Z] 10:39:32     INFO -  3676 INFO TEST-OK | mobile/android/components/extensions/test/mochitest/test_ext_tabs_update_url.html | took 827ms
[task 2020-10-06T10:39:32.493Z] 10:39:32     INFO -  3677 INFO TEST-START | mobile/android/components/extensions/test/mochitest/test_ext_webNavigation_onCommitted.html
[task 2020-10-06T10:39:32.494Z] 10:39:32     INFO -  3678 INFO TEST-OK | mobile/android/components/extensions/test/mochitest/test_ext_webNavigation_onCommitted.html | took 210ms
[task 2020-10-06T10:39:32.494Z] 10:39:32     INFO -  3679 INFO TEST-START | Shutdown
[task 2020-10-06T10:39:32.494Z] 10:39:32     INFO -  3680 INFO Passed:  173
[task 2020-10-06T10:39:32.494Z] 10:39:32  WARNING -  3681 INFO Failed:  6
[task 2020-10-06T10:39:32.494Z] 10:39:32  WARNING -  One or more unittests failed.
[task 2020-10-06T10:39:32.495Z] 10:39:32     INFO -  3682 INFO Todo:    1
[task 2020-10-06T10:39:32.495Z] 10:39:32     INFO -  3683 INFO Mode:    e10s
[task 2020-10-06T10:39:32.495Z] 10:39:32     INFO -  3684 INFO Slowest: 329611ms - /tests/mobile/android/components/extensions/test/mochitest/test_ext_tabs_create.html
[task 2020-10-06T10:39:32.495Z] 10:39:32     INFO -  3685 INFO SimpleTest FINISHED

Flags: needinfo?(gijskruitbosch+bugs)

Phabricator Automation

Updated

•

4 years ago

Attachment #9179746 - Attachment description: Bug 1668586 - stop messing with web progress listeners when changing remoteness, r?mattwoodrow! → Bug 1668586 - stop messing with web progress listeners when changing remoteness, r?mattwoodrow!,agi!

:Gijs (he/him)

Assignee

Updated

•

4 years ago

Flags: needinfo?(gijskruitbosch+bugs)

Pulsebot

Comment 11

•

4 years ago

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/569eb3fda698 stop messing with web progress listeners when changing remoteness, r=mattwoodrow,geckoview-reviewers,agi

Atila Butkovits

Comment 12

•

4 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/569eb3fda698

Status: NEW → RESOLVED

Closed: 4 years ago

status-firefox83: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → 83 Branch

Julien Cristau [:jcristau]

Comment 13

•

4 years ago

Is this bad enough we'd want to uplift into the last 82 beta, or should it ride the trains to 83?

Flags: needinfo?(gijskruitbosch+bugs)

:Gijs (he/him)

Assignee

Comment 14

•

4 years ago

(In reply to Julien Cristau [:jcristau] from comment #13)

Is this bad enough we'd want to uplift into the last 82 beta, or should it ride the trains to 83?

I think it's edge-casey enough that we probably don't need it in the last beta, and although it affects other dialogs I think the scenario in this bug is more likely to happen with print (preffed off by default in 82) than those, so on the whole I think we're fine not uplifting.

status-firefox82: affected → wontfix

Flags: needinfo?(gijskruitbosch+bugs)

BMO Automation

Updated

•

1 year ago

Product: Toolkit → Toolkit Graveyard

aboutToRemote.gif 4 years ago Emil Ghitta, QA [:emilghitta] (deleted), image/gif		Details
Bug 1668586 - stop messing with web progress listeners when changing remoteness, r?mattwoodrow!,agi! 4 years ago :Gijs (he/him) (deleted), text/x-phabricator-request		Details