Closed Bug 1044600 Opened 5 years ago Closed 5 years ago

in-content preferences: empty dialogs after pressing backspace or the Back button

Categories

(Firefox :: Preferences, defect, P2)

defect
Points:
5

Tracking

()

RESOLVED FIXED
Firefox 39
Iteration:
39.2 - 23 Mar
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: soeren.hentzschel, Assigned: jaws)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached image screenshot
STR:

1. open a in-content preferences dialog
2. press the backspace key

Actual:

Empty dialog. See the screenshot.

Expected:

Nothing should happen.
Summary: in-content preferences: empty dialogs after pressing backspace → in-content preferences: empty dialogs after pressing backspace or the Back button
I think this is because of the iframe behaviour. Each time a dialog frame is loaded, the browser considers it as a new navigation state. I'm not sure there's a way to override that though.
Matt, do you know if there is an event that we should be preventing to stop the extra history entry? Note that after hitting Back and getting a blank dialog, you can hit Forward to repopulate the dialog.
Flags: needinfo?(MattN+bmo)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> Matt, do you know if there is an event that we should be preventing to stop
> the extra history entry? Note that after hitting Back and getting a blank
> dialog, you can hit Forward to repopulate the dialog.

You can try preventing the default behaviour of the iframe load event.
This is unrelated to global history, it's session history handled by the docshell. I'm not a big expert on that field.
https://developer.mozilla.org/en-US/docs/Web/Guide/API/DOM/Manipulating_the_browser_history
https://developer.mozilla.org/en-US/docs/Web/API/History

maybe you can use replaceState in some fancy way, or you can use the popstate event to detect the navigation and do something useful (like closing the popup)
Flags: needinfo?(mak77)
Flags: needinfo?(MattN+bmo)
OS: Mac OS X → All
Priority: -- → P2
Thanks Marco. We can look at adding a "popstate" event listener which checks to see if the page that was popped was the subdialog and if so close the dialog. That might break pressing the Forward button afterward, so we may need to look into what needs to be done to remove the forward-based history state.
Points: --- → 3
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
Attachment #8558217 - Flags: review?(gijskruitbosch+bugs)
Attachment #8558217 - Flags: review?(gijskruitbosch+bugs) → review+
Backed out in https://hg.mozilla.org/integration/fx-team/rev/81f1ca1e1a94 for causing intermittent timeouts in browser_change_app_handler.js.

Gijs, I pushed https://tbpl.mozilla.org/?tree=Try&rev=67595da5413a to tryserver to try to fix this but it still has the same issue. I think the failure is coming from the dialog being closed during the opening stage (as about:blank is unloaded). The patch pushed to try should work around this by only adding the unload event listener after the requested subdialog has been loaded and then removing it during unload. Do you see what could be causing this?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> Backed out in https://hg.mozilla.org/integration/fx-team/rev/81f1ca1e1a94
> for causing intermittent timeouts in browser_change_app_handler.js.
> 
> Gijs, I pushed https://tbpl.mozilla.org/?tree=Try&rev=67595da5413a to
> tryserver to try to fix this but it still has the same issue. I think the
> failure is coming from the dialog being closed during the opening stage (as
> about:blank is unloaded). The patch pushed to try should work around this by
> only adding the unload event listener after the requested subdialog has been
> loaded and then removing it during unload. Do you see what could be causing
> this?

Sorry for not having gotten to this within my usual time; I don't really understand this yet. I'll investigate later today; leaving the needinfo for now.
It seems this didn't help, and unfortunately I don't really have better ideas... :-\

The screenshot shows there's no dialog up when the test times out, but it's hard to tell why that is, and I can't personally reproduce the issue locally...
Flags: needinfo?(gijskruitbosch+bugs)
Iteration: 38.2 - 9 Feb → 38.3 - 23 Feb
(In reply to :Gijs Kruitbosch from comment #12)
> ct'd from bug 1130411:
> 
> remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4af91c43b595

This is green even after retriggers, so I guess that means we're relanding:

remote:   https://hg.mozilla.org/integration/fx-team/rev/c11a0930afc2
(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to :Gijs Kruitbosch from comment #12)
> > ct'd from bug 1130411:
> > 
> > remote:  
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=4af91c43b595
> 
> This is green even after retriggers, so I guess that means we're relanding:
> 
> remote:   https://hg.mozilla.org/integration/fx-team/rev/c11a0930afc2

Annnnnd backed out again because ~half the csets since that landing (including the landing itself) made browser_change_app_handler go orange again.

remote:   https://hg.mozilla.org/integration/fx-team/rev/8db110f4c10a

As noted on IRC yesterday, I wonder if the removed promiseDialogClosing's unload handler is at play here (I forgot to remove it in the trypush from comment #13).
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15)
> This is the try-push with the promiseDialogClosing:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4af91c43b595
> 
> This is a try-push without the promiseDialogClosing:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=caa526beb07b

Thanks! Sorry I didn't get to this yet even though I said I would. :-(
Landed with the closingPromise variable renamed and a comment alongside it.

https://hg.mozilla.org/integration/fx-team/rev/09a43afd57b2
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #17)
> Landed with the closingPromise variable renamed and a comment alongside it.
> 
> https://hg.mozilla.org/integration/fx-team/rev/09a43afd57b2

Doesn't the test failure here indicate an issue with the code that could be triggered with normal use? Note also that this is apparently still intermittently failing, just less frequently... :-(

https://treeherder.mozilla.org/logviewer.html#?job_id=2001625&repo=fx-team

It seems to me we should figure out *why* it's failing so frequently without that line and fix it (I'm hopeful that the failure condition we're still seeing is the same as the one that was happening more frequently without the 'loose promise'). I still suspect the "unload vs. close" loop because of the new unload handler that calls .close, and the pre-existing onclose handler that force-loads about:blank in the dialog.

Does this make sense or am I missing something?
Flags: needinfo?(jaws)
Depends on: 1130411
https://hg.mozilla.org/mozilla-central/rev/09a43afd57b2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Backed out for making bug 1130411 spike like crazy again.
https://hg.mozilla.org/integration/fx-team/rev/78fcab4b550e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 38 → ---
Depends on: 1135317
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
The trypush didn't fix the issue, as it was seen in Win7 Debug.
Trypush with some logging in each of the close-related functions of subdialog.js,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3717b3bfa50
I switched subdialog.js to use handleEvent instead of the multiple usages of addEventListener(function(){}.bind(this)), as well as added more logging. Note that treeherder only shows the full log for test runs that contain a failure. Successful test runs won't have the logging output to compare to. To get logging for a successful run, the test suite has to be run locally.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a38f599bdc91
Attached patch Patch using handleEvent (obsolete) — Splinter Review
And now super green on try. Good with you?
Attachment #8558217 - Attachment is obsolete: true
Attachment #8571547 - Flags: review?(gijskruitbosch+bugs)
Attachment #8571547 - Flags: review?(gijskruitbosch+bugs) → review+
sorry had to back this out since this might have caused https://treeherder.mozilla.org/logviewer.html#?job_id=2139328&repo=fx-team
Flags: needinfo?(jaws)
I've looked at browser_proxy_backup.js as well as other tests like it such as browser_connection.js and don't see any difference between the two that would cause one to leak and other to not leak. There are three tests that use this pattern of a windowWatcher: browsre_proxy_backup.js, browser_connection.js, and browser_connection_bug388287.js.

I imagine it has something to do with acceptDialog being called, which is what the test does to close the dialog, but I'm not sure. I thought the leak may have been coming from the windowWatcher that wasn't being unregistered but the test finishes and the code path to finish the test unregisters the windowWatcher.

Gijs, any ideas here?
Flags: needinfo?(gijskruitbosch+bugs)
Whiteboard: [fixed-in-fx-team]
Not sure offhand but I can at least reproduce locally with a debug build.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #31)
> Some attempts here, I think it might be close if we fix the
> browser_subdialog bustage?
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8656f8e4368d
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc01090f91cc
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b655249f2aa3

Sounds good!
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8574788 [details] [diff] [review]
Patch using handleEvent and removeEventListener

Review of attachment 8574788 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/in-content/subdialogs.js
@@ +111,5 @@
> +      case "unload":
> +        this._onUnload(aEvent);
> +        break;
> +      default:
> +        break;

default: break; seems useless. :-)
Attachment #8574788 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/e1d6e24248d2
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Comment on attachment 8574788 [details] [diff] [review]
Patch using handleEvent and removeEventListener

Approval Request Comment
[Feature/regressing bug #]: n/a  /// in-content-prefs
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]: ample automated tests
[Risks and why]: this will cause bug 1130411 to start happening on aurora. When this lands, we should disable browser_change_app_handler.js for that tree (for now). I'm working to fix the automated test, but normal behaviour is not affected, and this patch blocks other patches that also need uplift for 38, as well as fixing issues that would likely be able to cause memory leaks. I'd rather have them on 38 sooner than later.
[String/UUID change made/needed]: nope
Attachment #8574788 - Flags: approval-mozilla-aurora?
Comment on attachment 8574788 [details] [diff] [review]
Patch using handleEvent and removeEventListener

reset the status-firefox38 flag as it appears to have been set to 'fixed' by accident.  approving for uplift to 38.
Attachment #8574788 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.