Closed Bug 1668586 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit Graveyard :: Notifications and Alerts, defect)

Product:
Toolkit Graveyard
Component:
Notifications and Alerts
Type:
defect

Tracking

(firefox82 wontfix, firefox83 fixed)

Status:
RESOLVED FIXED
Milestone:
83 Branch
Tracking Flags:
Tracking Status
firefox82 --- wontfix
firefox83 --- fixed

People

(Reporter: emilghitta, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

aboutToRemote.gif
2.42 MB, image/gif
Details
Bug 1668586 - stop messing with web progress listeners when changing remoteness, r?mattwoodrow!,agi!
47 bytes, text/x-phabricator-request
Details | Review
Attached image aboutToRemote.gif —

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

  1. Launch Firefox.
  2. Access the about:support page.
  3. Hit Ctrl + P.
  4. 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

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)

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

Whiteboard: [print2020_v82] [old-ui-]

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

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

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)
Has Regression Range: --- → no
Has STR: --- → yes
Assignee: nobody → gijskruitbosch+bugs

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

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

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)
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!
Flags: needinfo?(gijskruitbosch+bugs)
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

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

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)

(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)
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: