Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Description • 9 years ago

This argument was added in bug 498648. I think it's no longer useful. The original use case was for private browsing. With per-window PB, I think it's no longer needed.

I did some testing and I wasn't able to get multiple prompts to appear with my patch applied. I also examined the different paths on which we close/quit and it doesn't look like there's any way we would call PermitUnload twice for the same tab.

I was going to ask Ehsan to review this, but he's not reviewing anything now. Please feel free to pass this on to someone else Boris.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 9 years ago

Comment on attachment 8628533 [details] [diff] [review]
patch

I'm going to try Gijs here, because he's looked at this stuff recently and because it's not clear to me why we don't need this code anymore without spending some significant time tracing it....

Gijs, if it's not clear to you why this is ok please pass this back to me and I'll sort this out, ok?

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

Comment on attachment 8628533 [details] [diff] [review]
patch

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

(In reply to Bill McCloskey (:billm) from comment #0)
> I did some testing and I wasn't able to get multiple prompts to appear with
> my patch applied. I also examined the different paths on which we close/quit
> and it doesn't look like there's any way we would call PermitUnload twice
> for the same tab.

This is a very scary change. Did you trypush including mochitest-bc (and its e10s variant)? At least some of the bugs I fixed in this space have tests. Did you try all the steps from https://bugzilla.mozilla.org/show_bug.cgi?id=305085#c11 and https://bugzilla.mozilla.org/show_bug.cgi?id=305085#c12 ?

Is this change required for the e10s-ification of this stuff (ie does it make the e10s approach more difficult, or something), or just cleanup that you did because you saw it and thought we could get rid of it?

(In reply to Boris Zbarsky [:bz] from comment #1)
> Gijs, if it's not clear to you why this is ok please pass this back to me
> and I'll sort this out, ok?

Given that it's *not* abundantly clear to me why this is OK, passing this back.

Returning the review to bz because it isn't obvious to me why this is OK now.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 3 • 9 years ago

Comment on attachment 8628533 [details] [diff] [review]
patch

Bah, I hadn't seen bug 305085 before and I think I missed running the test since it's not enabled with e10s.

This patch makes bug 967873 a lot easier. I'm hoping there's still some way to get rid of this code.

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

(In reply to Bill McCloskey (:billm) from comment #3)
> Comment on attachment 8628533 [details] [diff] [review]
> patch
> 
> Bah, I hadn't seen bug 305085 before and I think I missed running the test
> since it's not enabled with e10s.

Yes. We come full circle, because the test doesn't work in e10s because there's no beforeunload implementation, which is what you're trying to fix... :-)

> This patch makes bug 967873 a lot easier. I'm hoping there's still some way
> to get rid of this code.

OK. If you want to talk through some of this, I'm happy to discuss on IRC/Vidyo/email/... :-)

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 5 • 9 years ago

I'm going to fold the work here into bug 967873. I couldn't find a way to separate them in the way I'd originally hoped.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 6 • 9 years ago

Lots of tests couldn't cope with a nested event loop running during removeTab. I hacked around this by passing in a parameter that avoids doing permitUnload in these cases.

Usually the problem was just that some event was received during the removeTab code and we hadn't installed a handler for it yet or something. Sometimes bad stuff happened if, for example, some lazy tabview initialization ran during the removeTab event loop. Another problem was tests that didn't use waitForExplicitFinish: if they spin a nested event loop, we seem to start running the next test before the previous one has finished.

A really awful e10s-related problem is that we sometimes send a sync message up to the parent to inform it of some event. If the parent calls removeTab while handling that event, then we'll deadlock. The child can't respond to the permitUnload message until it has gotten a reply to the sync message.

Mostly I was able to fix these issues by making the messages async since they didn't actually need to be sync. In one or two cases I had to run the removeTab call based off of executeSoon. I looked at all the places where we call removeTab in production code and it doesn't seem like this can happen. Add-ons can trigger it unfortunately. In the worst case, that should trigger a timeout after 5 seconds.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 7 • 9 years ago

Comment on attachment 8643404 [details] [diff] [review]
test changes

Oops, wrong bug.