Sören Hentzschel Reporter
	Description • 10 years ago

STR:

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

Actual:

Empty dialog. See the screenshot.

Expected:

Nothing should happen.

	Tim Nguyen :ntim
	Comment 2 • 10 years ago

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.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 3 • 10 years ago

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.

	Tim Nguyen :ntim
	Comment 4 • 10 years ago

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

	Marco Bonardo [:mak] (Away Apr 25 - May 5)
	Comment 5 • 10 years ago

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)

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 6 • 9 years ago

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.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 8 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/756dc51160cf

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 9 • 9 years ago

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?

	:Gijs (he/him)
	Comment 10 • 9 years ago

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

	:Gijs (he/him)
	Comment 11 • 9 years ago

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

	:Gijs (he/him)
	Comment 12 • 9 years ago

ct'd from bug 1130411:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=4af91c43b595

	:Gijs (he/him)
	Comment 13 • 9 years ago

(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

	:Gijs (he/him)
	Comment 14 • 9 years ago

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

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 15 • 9 years ago

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

	:Gijs (he/him)
	Comment 16 • 9 years ago

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

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 17 • 9 years ago

Landed with the closingPromise variable renamed and a comment alongside it.

https://hg.mozilla.org/integration/fx-team/rev/09a43afd57b2

	:Gijs (he/him)
	Comment 18 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 19 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/09a43afd57b2

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 9 years ago

Backed out for making bug 1130411 spike like crazy again.
https://hg.mozilla.org/integration/fx-team/rev/78fcab4b550e

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 9 years ago

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/78fcab4b550e

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 22 • 9 years ago

Potential fix for the orange,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=32aac60b0fff

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 23 • 9 years ago

The trypush didn't fix the issue, as it was seen in Win7 Debug.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 24 • 9 years ago

Trypush with some logging in each of the close-related functions of subdialog.js,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3717b3bfa50

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 25 • 9 years ago

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

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 26 • 9 years ago

And now super green on try. Good with you?

	Carsten Book [:Tomcat]
	Comment 27 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/e440ab8f0a51

	Carsten Book [:Tomcat]
	Comment 28 • 9 years ago

sorry had to back this out since this might have caused https://treeherder.mozilla.org/logviewer.html#?job_id=2139328&repo=fx-team

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 29 • 9 years ago

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?

	:Gijs (he/him)
	Comment 30 • 9 years ago

Not sure offhand but I can at least reproduce locally with a debug build.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 31 • 9 years ago

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

	:Gijs (he/him)
	Comment 32 • 9 years ago

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

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 33 • 9 years ago

Green on try,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a614ca03f81b

	:Gijs (he/him)
	Comment 34 • 9 years ago

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. :-)

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 35 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/e1d6e24248d2

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 36 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/e1d6e24248d2

	:Gijs (he/him)
	Comment 37 • 9 years ago

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

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 38 • 9 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 39 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/6640b75100f8