Closed Bug 1336763 Opened 7 years ago Closed 7 years ago

Only message for permitUnload on tabs that have indicated that they contain a frame that has a beforeunload handler

Categories

(Core :: DOM: Content Processes, defect, P1)

Product:
Core
Component:
DOM: Content Processes
Version:
50 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Iteration:
55.4 - May 1
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-performance])

Attachments

(7 files, 1 obsolete file)

Bug 1336763 - Add a hasBeforeUnload attribute to nsITabParent.
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1336763 - Don't ask content process for permission to unload a window if it never set an onbeforeunload event handler.
59 bytes, text/x-review-board-request
dao
: review+
Details
Bug 1336763 - Regression tests that exercise nsITabParent's hasBeforeUnload attribute.
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1336763 - If browser.permitUnload times out in CanCloseWindow, make sure to check the rest of the browsers belonging to other processes before returning a result.
59 bytes, text/x-review-board-request
dao
: review+
Details
Bug 1336763 - Update browser_beforeunload_between_chrome_content.js to use ContentTask.
59 bytes, text/x-review-board-request
jessica
: review+
Details
Bug 1336763 - Don't wait for response from content process in browser_closeTab.js UITour test to avoid a timeout.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1336763 - Wait for the browser to say it can go back before going back in browser_grouped_shistory_dead_navigate.js test.
59 bytes, text/x-review-board-request
nika
: review+
Details 
We do this thing right now in remote-browser.xml where we send an async message down to content to run permitUnload on a tab. permitUnload fires the beforeunload event, and gives web pages an opportunity to warn the user about leaving the page.

While we send that message, we spin an event loop in the parent (ugh), since the parent wants to know if the permitUnload can happen synchronously. That's the way permitUnload was originally designed.

If we don't hear back from content within 5 seconds, we "time out" the permit unload, and I believe we end up just closing the tab or window.

If a content process is bogged down or hung, this is bad. The user might be trying to close a misbehaving tab, and we should probably let them instead of getting in their way.

One thing we could do that is "easy" is only do the permitUnload stuff on tabs where we know that one of the subdocuments has added a beforeunload event handler. For the rest of them, we know that they can be unloaded immediately.

That seems like a pretty easy win.

For the pages that _do_ have beforeunload event handlers, that's a trickier situation - but let's not worry about those right now. Let's take care of the easy case first.
Hi Mike,
Not sure if you've seen the discussion in other bugs about this. But we have to be careful not to have races where a page adds a beforeunload handler that doesn't run because the user closed the tab before the parent was notified that it exists.
Perhaps we could do something like this:

Use has a window with some tabs. User clicks on tab that the parent thinks has not set an onbeforeunload handler, and it closes immediately. Meanwhile, that tab's content has set an onbeforeunload handler.

Content eventually reacts to the closure of the tab by firing the onbeforeunload event, and the content process is informed that the page wants to ask the user if it still wants to stay on the page. At this point, since the TabChild is aware that it's been closed already in the parent, asks the parent if it should be re-opened.

TabParent receives the message, and then uses the OS's notification system to ask the user if it wants to revive a tab that's asking if it should really close.

Same could happen for multiple tabs if the user closes the window.

The quit case is harder though.
ehsan had another idea which might be even better:

Have a shared memory region between the parent and content process that maps tab ID to a bool that says "I just added a beforeunload handler" that the content process takes a lock and sets on page load (set to false), and takes another lock and sets when such an event handler is set (set to true). The parent can then read that bit to know that it's necessary to message the content process to run the handler.

That would give us immediate tab close for tabs that had never set the event handlers at the time of unload, and the wait for the tabs that have.
Do you think the idea in comment 3 is sufficient to sidestep most of the race concerns?
Flags: needinfo?(wmccloskey)
Here's a solution that makes sense to me: when the user closes a tab, send a message to the hang monitor thread in the child asking it to terminate any scripts from that tab if they run for more than 500ms (or some short threshold).

If we do that, then I suspect that most people's complaints will go away.
Flags: needinfo?(wmccloskey)
In my profiling these days I see quite a bit of cases where the content process gets blocked on other things.  In such cases with your suggestion there will still be a 500ms delay for tab closing which is quite perceivable.  This will happen on pages that may already behave sluggish, giving the impression of the page making the whole browser sluggish, which I think is really unacceptable.  How about the hang monitor thread checking whether the page is running scripts, and in case it isn't immediately return with information about whether the beforeunload handler is registered?

I would also say that for pages that are busy running scripts we should use some timeout that is much lower, that isn't perceivable.  Such as 50ms?
Blocks: 1344302
No longer blocks: 1336241
I talked to ehsan about this today.

So the reason we're jumping through all of these hoops is because we believe there is a risk of user data loss if we close the tab / destroy the TabChild during a window of time where the content has (or is about to) set a beforeunload handler, but the parent hasn't heard about it. That's what billm is talking about in comment 1.

ehsan's argument was that yes, this is possible in theory - but in practice, he doesn't believe it actually happens.

I feel like this is something we could collect data about. Here's what I propose:

1) Have the TabChild tell the TabParent asynchronously when a beforeunload event handler has been set.
2) When the remote-browser.xml's permitUnload is called, have the message it sends down include the TabParent's opinion on whether or not a beforeunload handler has been set.
3) When the content process sees that message, have it compare TabParent's opinion with reality. If they don't match, we should either:
  3.1) Diagnostic-crash with some annotation so that it shows up in crash-stats
  3.2) Bump a counter in Telemetry for a probe that we add here.

In either case, this should give us some data on whether or not the TabParent being out of sync with the TabChild on beforeunload is common. We _know_ it's possible, I think the question is how much we actually need to care.
I agree with the plan obviously.  :-)  Bill, what do you think?
Flags: needinfo?(wmccloskey)
Whiteboard: [qf:p1]
I thought about this some more.

It still seems important to me that we run the onbeforeunload handler if a page has requested it. That's the only way for the page to save its final data and do XHRs before it's closed. However, I don't think there's any need to worry about the "Are you sure you want to close this tab" dialog. That's mostly just an annoyance. So I think it's fine to have some racy state as long as we only use it to decide whether to display the dialog or not. And, if we're not going to display the dialog, we might as well close the tab right away.

However, we would still need to decide what to do about killing scripts. I'm worried about setting a threshold as low as 50ms. Even if we detect the page is running JS, it could have just ended up in a full GC. That would mean that, through no fault of its own, it would get abruptly terminated.

We could stick with our existing 10s slow script timer, but that doesn't seem aggressive enough if the user has just closed the tab. Additionally, it would be confusing for a tab that was already closed to continue to cause the process to be hung.

So perhaps we could kill the page's scripts using a more aggressive timer if its tab has already been closed. I think 1 or 2 seconds would be reasonable. And we wouldn't show any UI for that.

If you guys still want to discuss this, we should meet about it.
Flags: needinfo?(wmccloskey)
Assignee: nobody → mconley
FWIW I'd feel a lot less strongly about how long we keep the page running after we've closed the page letting it finish whatever it needs to do, giving it more time than 50ms if the user wouldn't be able to tell sounds fine to me..
(In reply to :Ehsan Akhgari (busy) from comment #10)
> FWIW I'd feel a lot less strongly about how long we keep the page running
> after we've closed the page letting it finish whatever it needs to do,
> giving it more time than 50ms if the user wouldn't be able to tell sounds
> fine to me..

Well the problem is that it will still affect other tabs running in the same process.
I agree that the beforeunload / unload scripts that we run when (invisibly) destroying the tab in the content process (after the parent has made it visibly closed) might affect other tabs running in the same process.

I kinda feel like that's separate though. Like, unless I'm mistaken, that's already an issue. What I'm really concerned about in this bug _is_ the dialog part, since that's what the parent is spinning the event loop waiting on.

So here's what I propose:

1) Throw out or defer the plan to add probes for the "race" case where the content process and parent process are out of sync on whether or not a beforeunload event handler has been set (comment 7)
2) Do the work of having the content process inform the parent when beforeunload handlers are set in some frame in a tab.
3) If the parent is attempting to close a tab and it thinks there's no beforeunload handler set, just close the window. Send the content process the async message saying that we want to destroy the TabChild.
4) File a follow-up bug to tune our tolerance on long running beforeunload / unload event handlers so that we reduce how much it affects other tabs.

Sound good?
Comment 12 sound like an okay way forward?
Flags: needinfo?(wmccloskey)
Flags: needinfo?(ehsan)
Sounds fine to me.
Flags: needinfo?(wmccloskey)
s/close the window/close the tab
ehsan digs it, via IRC.
Flags: needinfo?(ehsan)
See bug 1350060 for the bug on tuning our tolerance on long-running beforeunload / unload event scripts.
See Also: → 1350060
We are now collecting telemetry on permitUnload now: https://mzl.la/2na3fo3
Got some regression stuff off of my plate, and I've started working on this now.
This is pretty poor, but it's where I'm starting.

Questions:

1) I'm not sure if EventListenerManager is the right place to notice the beforeunload events. Should I do that in nsGlobalWindow's AddEventListener methods instead? Same for RemoveEventListener.

2) When an inner window goes away (perhaps it's in an <iframe> that's been removed from the DOM, or perhaps it's navigated forward, etc), I want to be able to determine how many beforeunload event handlers had been registered, and inform the TabChild that it can decrement by that amount. Apparently, FreeInnerObjects is too late to do that, since the nsDocShell will have been unhooked from the docshell tree at that point, and we've got no way to get to the TabChild. Is there a better, centralized place to notice when an inner window like this "goes away"?

ni'ing ehsan, who talked to me today about this. Sorry for loading more on.
Flags: needinfo?(ehsan)
(In reply to Mike Conley (:mconley) from comment #22)
> 1) I'm not sure if EventListenerManager is the right place to notice the
> beforeunload events. Should I do that in nsGlobalWindow's AddEventListener
> methods instead? Same for RemoveEventListener.

ELM looks fine to me!  But please double check with Olli.

> 2) When an inner window goes away (perhaps it's in an <iframe> that's been
> removed from the DOM, or perhaps it's navigated forward, etc), I want to be
> able to determine how many beforeunload event handlers had been registered,
> and inform the TabChild that it can decrement by that amount. Apparently,
> FreeInnerObjects is too late to do that, since the nsDocShell will have been
> unhooked from the docshell tree at that point, and we've got no way to get
> to the TabChild. Is there a better, centralized place to notice when an
> inner window like this "goes away"?

Hmm, I'm half guessing what's happening based on applying your patch and building and running it in my head.  :-)  Is this happening from the nsGlobalWindow::DetachFromDocShell() caller but not from the nsGlobalWindow::SetNewDocument() caller?  If yes, I think we can check for mIsBeingDestroyed, and add a special code path earlier in nsDocShell::Destroy before we get detached from the docshell tree.
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #23)
> (In reply to Mike Conley (:mconley) from comment #22)
> > 1) I'm not sure if EventListenerManager is the right place to notice the
> > beforeunload events. Should I do that in nsGlobalWindow's AddEventListener
> > methods instead? Same for RemoveEventListener.
> 
> ELM looks fine to me!  But please double check with Olli.

ni?ing smaug - is the ELM the right place for this? Essentially, I just want a smart place to detect when a beforeunload listener has been added or removed from a window. Or are the add/remove event listener functions in nsGlobalWindow a better place?
Flags: needinfo?(bugs)
Do something here http://searchfox.org/mozilla-central/rev/944f87c575e8a0bcefc1ed8efff10b34cf7a5169/dom/base/nsGlobalWindow.cpp#13440 and if you need to handle removal, override also
EventListenerRemoved(nsIAtom* aType).
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #25)
> Do something here
> http://searchfox.org/mozilla-central/rev/
> 944f87c575e8a0bcefc1ed8efff10b34cf7a5169/dom/base/nsGlobalWindow.cpp#13440
> and if you need to handle removal, override also
> EventListenerRemoved(nsIAtom* aType).

Yeah, that looks like the perfect spot, thanks.

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #23)
> Hmm, I'm half guessing what's happening based on applying your patch and
> building and running it in my head.  :-)  Is this happening from the
> nsGlobalWindow::DetachFromDocShell() caller but not from the
> nsGlobalWindow::SetNewDocument() caller?  If yes, I think we can check for
> mIsBeingDestroyed, and add a special code path earlier in
> nsDocShell::Destroy before we get detached from the docshell tree.

Still too late, I'm afraid - in the browsing case, by the time nsDocShell::Destroy is called (off of the frameloader destruction runnable), the mTreeParent can already be set to nullptr due to a call to nsDocShell::DestroyChildren on the top-level nsDocShell due to setting up a new viewer for a top-level navigation.

I'm going to try to come up with something a little more robust...
Status: NEW → ASSIGNED
Flags: qe-verify?
Whiteboard: [qf:p1] → [photon-performance] [qf:p1]
So I've taken a solution suggested by mystor and combined it with smaug's advice in comment 25. Essentially, I add a weak TabChild pointer to the ELM when the first beforeunload event listener is added. When we RemoveAllListeners (which is a thing that we do when we are ::Disconnect'ed), I get at that weak TabChild, and iterate the listeners, decrementing the TabChild beforeunload count for each beforeunload listener we find. If the TabChild is already destroyed, I guess we don't really care about informing it.

The TabChild keeps the count, which seemed simplest rather than trying to wire in any kind of beforeunload awareness in the nsIDocShell* structure.

Not sure if we're worried about the overhead of adding the weak pointer to ELM. If push comes to shove, we could stuff that into nsGlobalWindow instead, as per mystor's suggestion.

I also need to add tests for this, but I thought I'd post it for a first round of review feedback, since this is the direction I'm going.
Attachment #8858159 - Flags: review?(dtownsend)
Having thought about it some more, I think there's a better way to do this. permitUnload is run against each tab when closing the whole window[1], and I also want to update that to skip the ones without hasBeforeUnload. Otherwise, they'll be called serially, meaning that one hung tab can delay the closing of the rest of the tabs.

[1]: http://searchfox.org/mozilla-central/rev/d4eaa9c2fa54d553349ac88f0c312155a4c6e89e/browser/base/content/browser.js#6427-6435
(In reply to Mike Conley (:mconley) from comment #30)
> Having thought about it some more, I think there's a better way to do this.
> permitUnload is run against each tab when closing the whole window[1], and I
> also want to update that to skip the ones without hasBeforeUnload.

Using hasBeforeUnload in said code sounds right to me. Basically, we should never call permitUnload without that check.

> Otherwise, they'll be called serially, meaning that one hung tab can delay
> the closing of the rest of the tabs.

I'm not sure what you mean here. Closing a window doesn't close tabs individually (in the sense that it would call gBrowser.removeTab). I don't think we want to change this.

> [1]:
> http://searchfox.org/mozilla-central/rev/
> d4eaa9c2fa54d553349ac88f0c312155a4c6e89e/browser/base/content/browser.
> js#6427-6435

Btw, why does this function return true as soon as a permitUnload call times out, regardless of what subsequent browsers might want? This seems wrong especially with multiple content processes.
(In reply to Dão Gottwald [::dao] from comment #31)
> > Otherwise, they'll be called serially, meaning that one hung tab can delay
> > the closing of the rest of the tabs.
> 
> I'm not sure what you mean here. Closing a window doesn't close tabs
> individually (in the sense that it would call gBrowser.removeTab). I don't
> think we want to change this.

Yeah, I misspoke - I should have said:

"Otherwise, they'll be called serially, meaning that one hung tab can delay checking if the rest of the tabs can close. We should only ever check the ones that might have a beforeunload handler set".

> 
> > [1]:
> > http://searchfox.org/mozilla-central/rev/
> > d4eaa9c2fa54d553349ac88f0c312155a4c6e89e/browser/base/content/browser.
> > js#6427-6435
> 
> Btw, why does this function return true as soon as a permitUnload call times
> out, regardless of what subsequent browsers might want? This seems wrong
> especially with multiple content processes.

Yeah, I agree that looks wrong. I'll see if I can address that here too.
Attachment #8855089 - Flags: review?(ehsan) → review?(wmccloskey)
Comment on attachment 8855089 [details]
Bug 1336763 - Add a hasBeforeUnload attribute to nsITabParent.

I gave a face to face feedback on this to Mike.  The gist is that the overall approach is fine, but I found the usage of the weak pointer to be very questionable.  He's going to work on another iteration using a normal strong ref.
Attachment #8855089 - Flags: review?(wmccloskey)
Iteration: --- → 55.4 - May 1
Flags: qe-verify? → qe-verify-
Tests coming up.
Attachment #8855089 - Flags: review?(ehsan)
Attachment #8859680 - Flags: review?(ehsan)
Bah - a number of failures here. In particular, it turns out I can't #include TabChild.h in here, since (I guess) part of it eventually brings in windows.h, and that's been banished in nsGlobalWindow.cpp (see bug 933099).

My workaround is to add my beforeUnloadAdded and beforeUnloadRemoved methods to nsITabChild instead.

I also have a number of crashes and failures to investigate. Thankfully, they're on Linux, so I have a shot at using rr to figure them out. Joy!
Comment on attachment 8859681 [details]
Bug 1336763 - If browser.permitUnload times out in CanCloseWindow, make sure to check the rest of the browsers belonging to other processes before returning a result.

https://reviewboard.mozilla.org/r/131690/#review134604

::: browser/base/content/browser.js:6438
(Diff revision 1)
>    });
>  
>    for (let browser of browsers) {
>      let {permitUnload, timedOut} = browser.permitUnload();
>      if (timedOut) {
> -      return true;
> +      continue;

The idea of this is that, if the user has 50 tabs open, they shouldn't have to wait 50*(timeout value) seconds. As Dao says, we could probably do better for multiple content processes. But I think that simply changing this to "continue" is a step in the wrong direction.
Attachment #8859681 - Flags: review?(wmccloskey) → review-
Comment on attachment 8859681 [details]
Bug 1336763 - If browser.permitUnload times out in CanCloseWindow, make sure to check the rest of the browsers belonging to other processes before returning a result.

https://reviewboard.mozilla.org/r/131690/#review134604

> The idea of this is that, if the user has 50 tabs open, they shouldn't have to wait 50*(timeout value) seconds. As Dao says, we could probably do better for multiple content processes. But I think that simply changing this to "continue" is a step in the wrong direction.

Okay, understood. One thing we could do is, while iterating the browsers, stash the osPid of any browser that times out, and skip permitUnload on browsers with the same osPid for the rest of the loop. Would that satisfy?
ni? to billm for comment 43.
Flags: needinfo?(wmccloskey)
Comment on attachment 8855089 [details]
Bug 1336763 - Add a hasBeforeUnload attribute to nsITabParent.

https://reviewboard.mozilla.org/r/126990/#review134670

Mike, thanks very much, this looks great!  Sorry for jumping in, MozReview UI is really confusing, it shows "r?ehsan" so I thought that I have a pending review request on both this and the patch and since I already had reviewed half of the patch I reviewed it and then only after I clicked Finish Review was that I realized there is no pending flag from me?!

Anyways, consider these drive-by nits.  :-)

::: dom/events/EventListenerManager.cpp:181
(Diff revision 4)
> +  if (mTarget) {
> +    uint32_t count = mListeners.Length();
> +    for (uint32_t i = 0; i < count; ++i) {
> +      Listener* listener = &mListeners.ElementAt(i);
> +      NotifyEventListenerRemoved(listener->mTypeAtom);
> +    }

I think it would be less error prone if you wrote this as:

  for (auto* listener : mListeners) {
    NotifyEventListenerRemoved(listener->mTypeAtom);
  }

::: dom/events/EventListenerManager.cpp:1392
(Diff revision 4)
>  EventListenerManager::Disconnect()
>  {
> -  mTarget = nullptr;
>    RemoveAllListeners();
> +  mTarget = nullptr;
>  }

Please add a comment here explaining that the order is important.
Comment on attachment 8859679 [details]
Bug 1336763 - Regression tests that exercise nsITabParent's hasBeforeUnload attribute.

https://reviewboard.mozilla.org/r/131686/#review134678

::: dom/tests/browser/browser_hasbeforeunload.js:271
(Diff revision 1)
> +
> +/**
> + * Tests that the nsITabParent hasBeforeUnload attribute works under
> + * a number of different scenarios. At a high-level, we test that
> + * hasBeforeUnload works properly during page / iframe navigation, or
> + * when an <iframe> with a beforeunload listener is removed from the DOM.

I spent some time thinking about other cases to test.

  * The behavior around replaceChild is probably interesting to test for completeness sake.
  * We should have tests for handling of sandboxed iframes, I was about to forget them.  :-(  In fact I think we have a bug in part 1 about it probably.  IINM sandboxed iframes can't run their onbeforeunload event listener due to sandboxing restrictions, so we shouldn't count them towards the number of onbeforeunload listeners.

::: testing/mochitest/BrowserTestUtils/content/content-task.js:11
(Diff revision 1)
>  
>  var {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
>  
>  Cu.import("resource://gre/modules/Task.jsm", this);
>  Cu.import("resource://testing-common/ContentTaskUtils.jsm", this);
> +Cu.import("resource://testing-common/TestUtils.jsm", this);

Could this be moved to browser_hasbeforeunload.js?  I don't think you actually need to expose this to the world? So this could go inside the function use in the ContentTask, for example.

I'm also not sure to what extent this matters.  So feel free to ignore me here if this isn't a big deal.  :-)
Attachment #8859679 - Flags: review?(ehsan) → review+
Comment on attachment 8858159 [details]
Bug 1336763 - Don't ask content process for permission to unload a window if it never set an onbeforeunload event handler.

https://reviewboard.mozilla.org/r/130102/#review134804

::: browser/base/content/browser.js:6432
(Diff revision 3)
> +  // at browsers that are non-remote or have the hasBeforeUnload
> +  // property on their nsITabParent set to true.
> +  let browsers = gBrowser.browsers.filter(browser => {
> +    let tabParent = browser.frameLoader.tabParent;
> +    return !browser.isRemoteBrowser ||
> +           (tabParent && tabParent.hasBeforeUnload);

A separate loop for this seems unnecessary and wasteful. Looks like this check should happen in the for-of loop.

Or is this needed at all? Doesn't the remote-browser.xml change to permitUnload take care of this?

::: toolkit/content/widgets/remote-browser.xml:309
(Diff revision 3)
>          <![CDATA[
> +          let { frameLoader } = this.QueryInterface(Components.interfaces.nsIFrameLoaderOwner);
> +          let tabParent = frameLoader.tabParent;
> +
> +          if (tabParent && !tabParent.hasBeforeUnload) {
> +            return { permitUnload: true, timedOut: false };

Shouldn't this check (!tabParent || !tabParent.hasBeforeUnload)? Why would tabParent be null anyway?
Attachment #8858159 - Flags: review?(dao+bmo) → review-
Comment on attachment 8855089 [details]
Bug 1336763 - Add a hasBeforeUnload attribute to nsITabParent.

https://reviewboard.mozilla.org/r/126990/#review134670

> I think it would be less error prone if you wrote this as:
> 
>   for (auto* listener : mListeners) {
>     NotifyEventListenerRemoved(listener->mTypeAtom);
>   }

I don't think nsAutoTObserverArray's can be iterated like this. I'm getting:

```
 0:15.01 c:/Users/mconley/mozilla/mozilla-central/dom/events/EventListenerManager.cpp(177): error C3312: no callable 'begin' function found for type 'nsAutoTObserverArray<mozilla::EventListenerManager::Listener,2>'
 0:15.02 c:/Users/mconley/mozilla/mozilla-central/dom/events/EventListenerManager.cpp(177): error C3312: no callable 'end' function found for type 'nsAutoTObserverArray<mozilla::EventListenerManager::Listener,2>'
 0:15.02 c:/Users/mconley/mozilla/mozilla-central/dom/events/EventListenerManager.cpp(178): error C2065: 'listener': undeclared identifier
 0:15.02 c:/Users/mconley/mozilla/mozilla-central/dom/events/EventListenerManager.cpp(178): error C2227: left of '->mTypeAtom' must point to class/struct/union/generic type
 0:15.02 c:/Users/mconley/mozilla/mozilla-central/dom/events/EventListenerManager.cpp(178): note: type is 'unknown-type'
 ```
Comment on attachment 8859679 [details]
Bug 1336763 - Regression tests that exercise nsITabParent's hasBeforeUnload attribute.

https://reviewboard.mozilla.org/r/131686/#review134678

> I spent some time thinking about other cases to test.
> 
>   * The behavior around replaceChild is probably interesting to test for completeness sake.
>   * We should have tests for handling of sandboxed iframes, I was about to forget them.  :-(  In fact I think we have a bug in part 1 about it probably.  IINM sandboxed iframes can't run their onbeforeunload event listener due to sandboxing restrictions, so we shouldn't count them towards the number of onbeforeunload listeners.

You're right!: http://searchfox.org/mozilla-central/rev/d8ac097a1de2544f0e51d186bc850662c5b7430a/dom/base/nsSandboxFlags.h#92-97

Thanks - will add the SANDBOXED_MODALS check and some tests for that case.

> Could this be moved to browser_hasbeforeunload.js?  I don't think you actually need to expose this to the world? So this could go inside the function use in the ContentTask, for example.
> 
> I'm also not sure to what extent this matters.  So feel free to ignore me here if this isn't a big deal.  :-)

Yeah, good call. Not sure why I put it in there. I guess I expected to use it in more ContentTask's, but that turned out not to be the case.
(In reply to Mike Conley (:mconley) from comment #43)
> Comment on attachment 8859681 [details]
> Bug 1336763 - If browser.permitUnload times out in CanCloseWindow, make sure
> to check the rest of the browsers before returning a result.
> 
> https://reviewboard.mozilla.org/r/131690/#review134604
> 
> > The idea of this is that, if the user has 50 tabs open, they shouldn't have to wait 50*(timeout value) seconds. As Dao says, we could probably do better for multiple content processes. But I think that simply changing this to "continue" is a step in the wrong direction.
> 
> Okay, understood. One thing we could do is, while iterating the browsers,
> stash the osPid of any browser that times out, and skip permitUnload on
> browsers with the same osPid for the rest of the loop. Would that satisfy?

Sure, that sounds fine. Although it might be nice to use this property instead of osPid:
http://searchfox.org/mozilla-central/source/dom/base/nsIMessageManager.idl#295
Flags: needinfo?(wmccloskey)
Comment on attachment 8858159 [details]
Bug 1336763 - Don't ask content process for permission to unload a window if it never set an onbeforeunload event handler.

https://reviewboard.mozilla.org/r/130102/#review136948

I don't see any change in this revision, previous comments still seem to apply
Attachment #8858159 - Flags: review?(dao+bmo) → review-
Whoops, sorry - I meant to clear the review flags. Still in the midst of cleaning this stuff up.

I'll clear the review flags now. Sorry about the noise.
Attachment #8859680 - Flags: review?(ehsan)
Comment on attachment 8855089 [details]
Bug 1336763 - Add a hasBeforeUnload attribute to nsITabParent.

This looks unchanged.  It looks like you have had a fight with MozReview and have been defeated.  :-)  We've all been there!
Attachment #8855089 - Flags: review?(ehsan)
Attachment #8859681 - Flags: review?(wmccloskey)
Comment on attachment 8855089 [details]
Bug 1336763 - Add a hasBeforeUnload attribute to nsITabParent.

https://reviewboard.mozilla.org/r/126990/#review134670

> Please add a comment here explaining that the order is important.

I talked to smaug, and ended up just putting a counter inside nsGlobalWindow.cpp. Now there are no changes to the ELM.
Comment on attachment 8858159 [details]
Bug 1336763 - Don't ask content process for permission to unload a window if it never set an onbeforeunload event handler.

https://reviewboard.mozilla.org/r/130102/#review134804

> A separate loop for this seems unnecessary and wasteful. Looks like this check should happen in the for-of loop.
> 
> Or is this needed at all? Doesn't the remote-browser.xml change to permitUnload take care of this?

Yeah, I actually don't think the separate loop is necessary. It _will_ be a nice optimization for things like multiple tab removals, but for a check like this, I don't actually think it's winning us much. I'll remove it.

> Shouldn't this check (!tabParent || !tabParent.hasBeforeUnload)? Why would tabParent be null anyway?

Yeah, good call. I can't think of a way that tabParent might not exist.
Comment on attachment 8859679 [details]
Bug 1336763 - Regression tests that exercise nsITabParent's hasBeforeUnload attribute.

I figure this has changed enough to warrant a re-review.

Interdiff: https://reviewboard.mozilla.org/r/131686/diff/1-3/
Attachment #8859679 - Flags: review+ → review?(ehsan)
Comment on attachment 8858159 [details]
Bug 1336763 - Don't ask content process for permission to unload a window if it never set an onbeforeunload event handler.

https://reviewboard.mozilla.org/r/130102/#review137232
Attachment #8858159 - Flags: review?(dao+bmo) → review+
Comment on attachment 8859681 [details]
Bug 1336763 - If browser.permitUnload times out in CanCloseWindow, make sure to check the rest of the browsers belonging to other processes before returning a result.

https://reviewboard.mozilla.org/r/131690/#review137234

::: browser/base/content/browser.js:6446
(Diff revision 3)
>      }
> +
>      if (!permitUnload) {
>        return false;
>      }
> +

nit: I wouldn't add a blank line here. Feel free to do it anyway if you prefer...
Attachment #8859681 - Flags: review?(dao+bmo) → review+
Attachment #8859680 - Attachment is obsolete: true
Attachment #8859680 - Flags: review?(ehsan)
Comment on attachment 8862574 [details]
Bug 1336763 - Wait for the browser to say it can go back before going back in browser_grouped_shistory_dead_navigate.js test.

https://reviewboard.mozilla.org/r/134430/#review137426
Attachment #8862574 - Flags: review?(michael) → review+
Comment on attachment 8855089 [details]
Bug 1336763 - Add a hasBeforeUnload attribute to nsITabParent.

https://reviewboard.mozilla.org/r/126990/#review137016

::: dom/base/nsGlobalWindow.cpp:2138
(Diff revisions 5 - 6)
> +
> +  if (mTabChild) {
> +    for (int32_t i = 0; i < mBeforeUnloadListenerCount; ++i) {
> +      mTabChild->BeforeUnloadRemoved();
> +    }
> +    mBeforeUnloadListenerCount = 0;

Wouldn't it be simpler to write this as:

  while (mBeforeUnloadListenerCount--) {
    mTabChild->BeforeUnloadRemoved();
  }

::: dom/base/nsGlobalWindow.cpp:13409
(Diff revisions 5 - 6)
>  
> -  if (aType == nsGkAtoms::onbeforeunload) {
> -    if (mTabChild) {
> +  if (aType == nsGkAtoms::onbeforeunload &&
> +      mTabChild &&
> +      (!mDoc || !(mDoc->GetSandboxFlags() & SANDBOXED_MODALS))) {
> +    mBeforeUnloadListenerCount++;
> +    MOZ_ASSERT(mBeforeUnloadListenerCount > 0);

You're performing an overflow check but as a debug-only assertion.  That seems wrong.  I suggest making the counter a 64-bit integer and the leave the assertions as is.

::: dom/base/nsGlobalWindow.cpp:13428
(Diff revisions 5 - 6)
>  {
> -  if (aType == nsGkAtoms::onbeforeunload) {
> -    if (mTabChild) {
> +  if (aType == nsGkAtoms::onbeforeunload &&
> +      mTabChild &&
> +      (!mDoc || !(mDoc->GetSandboxFlags() & SANDBOXED_MODALS))) {
> +    mBeforeUnloadListenerCount--;
> +    MOZ_ASSERT(mBeforeUnloadListenerCount >= 0);

Ditto here.

::: commit-message-9aacf:6
(Diff revision 6)
> +Bug 1336763 - Add a hasBeforeUnload attribute to nsITabParent. r?ehsan
> +
> +This will return true if any of the frames loaded in the associated
> +TabChild have set at least one onbeforeunload event handler. If those
> +handlers are all removed, or all of the documents with onbeforeunload
> +event handlers are unloaded, this becomes false again.

You should probably mention the sandbox exception here as well.

::: dom/base/nsGlobalWindow.h:1958
(Diff revision 6)
>    RefPtr<nsHistory>           mHistory;
>    RefPtr<mozilla::dom::CustomElementRegistry> mCustomElements;
>  
>    // These member variables are used on both inner and the outer windows.
>    nsCOMPtr<nsIPrincipal> mDocumentPrincipal;
> +  nsCOMPtr<nsITabChild>  mTabChild;

Please document that this is only valid in the content process (even though it may be obvious.)

::: dom/base/nsGlobalWindow.h:2047
(Diff revision 6)
>  
>    // When non-zero, the document should receive a vrdisplayactivate event
>    // after loading.  The value is the ID of the VRDisplay that content should
>    // begin presentation on.
>    uint32_t mAutoActivateVRDisplayID; // Outer windows only
> +  int32_t mBeforeUnloadListenerCount; // Inner and outer windows

Why make this signed?

Why do you need this on both inners and outers?  I think you should probably use outer, no?

::: dom/base/nsGlobalWindow.cpp:2364
(Diff revision 6)
>    }
>    NS_IMPL_CYCLE_COLLECTION_UNLINK(mSuspendedDoc)
>    NS_IMPL_CYCLE_COLLECTION_UNLINK(mIndexedDB)
>    NS_IMPL_CYCLE_COLLECTION_UNLINK(mDocumentPrincipal)
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mTabChild)
> +

Nit: please remove the blank line.

::: dom/base/nsGlobalWindow.cpp:13409
(Diff revision 6)
>  
> +  if (aType == nsGkAtoms::onbeforeunload &&
> +      mTabChild &&
> +      (!mDoc || !(mDoc->GetSandboxFlags() & SANDBOXED_MODALS))) {
> +    mBeforeUnloadListenerCount++;
> +    MOZ_ASSERT(mBeforeUnloadListenerCount > 0);

You're adding an overflow check here as an assertion which seems wrong.  I suggest making the counter a 64-bit integer so you can actually not worry about the overflow case, and then leave the assertions untouched.

::: dom/base/nsGlobalWindow.cpp:13428
(Diff revision 6)
> +{
> +  if (aType == nsGkAtoms::onbeforeunload &&
> +      mTabChild &&
> +      (!mDoc || !(mDoc->GetSandboxFlags() & SANDBOXED_MODALS))) {
> +    mBeforeUnloadListenerCount--;
> +    MOZ_ASSERT(mBeforeUnloadListenerCount >= 0);

Ditto.

::: dom/ipc/TabChild.h:783
(Diff revision 6)
>    RefPtr<nsIContentChild> mManager;
>    RefPtr<TabChildSHistoryListener> mHistoryListener;
>    uint32_t mChromeFlags;
>    int32_t mActiveSuppressDisplayport;
>    uint64_t mLayersId;
> +  int32_t mBeforeUnloadListeners;

Perhaps this needs to be a 64-bit counter as well?
Attachment #8855089 - Flags: review?(ehsan) → review-
Comment on attachment 8855089 [details]
Bug 1336763 - Add a hasBeforeUnload attribute to nsITabParent.

https://reviewboard.mozilla.org/r/126990/#review137016

> Wouldn't it be simpler to write this as:
> 
>   while (mBeforeUnloadListenerCount--) {
>     mTabChild->BeforeUnloadRemoved();
>   }

Yep, will do!

> Why make this signed?
> 
> Why do you need this on both inners and outers?  I think you should probably use outer, no?

Actually, if I understand correctly, [the ELM is hosted in the inner window and shared with the outer](http://searchfox.org/mozilla-central/rev/068e6f292503df13515a52321fc3844e237bf6a9/dom/base/nsGlobalWindow.cpp#10296-10306), so really this should be "inner only". I'll update it.

Regarding making mBeforeUnloadListenerCount be signed - I was doing this so I could more easily detect imbalances ("too many beforeunloads have been removed - more than have been added!"), and the overflow check was a bonus.

Do you think it's unnecessary to worry about the imbalance? Or should I instead use a signed 64-bit integer?
ni? to ehsan for comment 82.
Flags: needinfo?(ehsan)
(In reply to Mike Conley (:mconley) - PTO on April 28th. from comment #82)
> > Why make this signed?
> > 
> > Why do you need this on both inners and outers?  I think you should probably use outer, no?
> 
> Actually, if I understand correctly, [the ELM is hosted in the inner window
> and shared with the
> outer](http://searchfox.org/mozilla-central/rev/
> 068e6f292503df13515a52321fc3844e237bf6a9/dom/base/nsGlobalWindow.cpp#10296-
> 10306), so really this should be "inner only". I'll update it.

Oh, right, yes.

> Regarding making mBeforeUnloadListenerCount be signed - I was doing this so
> I could more easily detect imbalances ("too many beforeunloads have been
> removed - more than have been added!"), and the overflow check was a bonus.
> 
> Do you think it's unnecessary to worry about the imbalance? Or should I
> instead use a signed 64-bit integer?

If you go 64-bit, if doesn't matter either way.  :-)
Flags: needinfo?(ehsan)
Comment on attachment 8859679 [details]
Bug 1336763 - Regression tests that exercise nsITabParent's hasBeforeUnload attribute.

https://reviewboard.mozilla.org/r/131686/#review137578
Attachment #8859679 - Flags: review?(ehsan) → review+
Comment on attachment 8862572 [details]
Bug 1336763 - Update browser_beforeunload_between_chrome_content.js to use ContentTask.

https://reviewboard.mozilla.org/r/134426/#review137604
Attachment #8862572 - Flags: review?(jjong) → review+
Comment on attachment 8855089 [details]
Bug 1336763 - Add a hasBeforeUnload attribute to nsITabParent.

https://reviewboard.mozilla.org/r/126990/#review137016

> Yep, will do!

Ended up doing a slight variation:

``` c++
    while (mBeforeUnloadListenerCount-- > 0) {
      mTabChild->BeforeUnloadRemoved();
    }
```

Since mBeforeUnloadListener might start at 0.
Comment on attachment 8855089 [details]
Bug 1336763 - Add a hasBeforeUnload attribute to nsITabParent.

https://reviewboard.mozilla.org/r/126990/#review137766

::: dom/base/nsGlobalWindow.h:2048
(Diff revisions 6 - 7)
>  
>    // When non-zero, the document should receive a vrdisplayactivate event
>    // after loading.  The value is the ID of the VRDisplay that content should
>    // begin presentation on.
>    uint32_t mAutoActivateVRDisplayID; // Outer windows only
> -  int32_t mBeforeUnloadListenerCount; // Inner and outer windows
> +  int64_t mBeforeUnloadListenerCount; // Inner windows only

Please have assertions that you are dealing with inner windows everywhere this is used.
Attachment #8855089 - Flags: review?(ehsan) → review+
Comment on attachment 8862573 [details]
Bug 1336763 - Don't wait for response from content process in browser_closeTab.js UITour test to avoid a timeout.

https://reviewboard.mozilla.org/r/134428/#review137782
Attachment #8862573 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8855089 [details]
Bug 1336763 - Add a hasBeforeUnload attribute to nsITabParent.

https://reviewboard.mozilla.org/r/126990/#review137766

> Please have assertions that you are dealing with inner windows everywhere this is used.

Done. I hope MOZ_ASSERT was okay. I didn't add one in FreeInnerObjects, since that already has an IsInnerWindow assertion near the beginning of the function.
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f588b8806722
Add a hasBeforeUnload attribute to nsITabParent. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/1c591da5bb1d
Regression tests that exercise nsITabParent's hasBeforeUnload attribute. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/fa1e0609ba33
Don't ask content process for permission to unload a window if it never set an onbeforeunload event handler. r=dao
https://hg.mozilla.org/integration/autoland/rev/e74b968f87cb
If browser.permitUnload times out in CanCloseWindow, make sure to check the rest of the browsers belonging to other processes before returning a result. r=dao
https://hg.mozilla.org/integration/autoland/rev/f85469165de7
Update browser_beforeunload_between_chrome_content.js to use ContentTask. r=jessica
https://hg.mozilla.org/integration/autoland/rev/5ff4a271f19e
Don't wait for response from content process in browser_closeTab.js UITour test to avoid a timeout. r=MattN
https://hg.mozilla.org/integration/autoland/rev/c30cfe319b6d
Wait for the browser to say it can go back before going back in browser_grouped_shistory_dead_navigate.js test. r=mystor
This has caused a regression in lazy browser tabs and causes all lazy browsers to be instantiated when window closes: bug 1360940.
Sounds like we need a regression test for that.
Depends on: 1360940
Depends on: 1363614
See Also: → 1406782
See Also: → 1543251
Performance Impact: --- → P1
Whiteboard: [photon-performance] [qf:p1] → [photon-performance]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: