986935 - Restore from crash page is blank if default client dialog is open

Status: RESOLVED FIXED

(SeaMonkey :: Session Restore, defect)

Description

[[e-mail]]

Steps to reproduce problem:
1. Find page that crashes in SeaMonkey
2. Restart SeaMonkey to find that it still crashes
3. Start a debug build of SeaMonkey to troubleshoot

At this point the debug build might throw up a default client dialog at the same time that you get the restore from crash page. Unfortunately the default client dialog is modal and this causes events to get suppressed on the owner, i.e. the browser window. One of these events is the load event for the about:sessionrestore page which therefore cannot find the session to restore.

As a workaround, having dismissed the default client dialog you can click reload and the about:sessionrestore page will have its session restored and will populate itself.

Comment 1

[[e-mail]]

Attachment: Workaround

This version works around the problem by adding the load event listener before broadcasting the window restored notification.

Comment 2

[[e-mail]]

Attachment: With lots of bind and dispatch

By dispatching the original notification this allows control to return and the event listeners to be added before the default client dialog is opened.

While I was there I switched everything that was using zero timeouts to dispatched events instead.

While I was there I switched everything that was using _this to bind instead.

Comment 3

[Philip Chee]

Comment on attachment 8395429 [details] [diff] [review]
With lots of bind and dispatch

This works for me.

> -XPCOMUtils.defineLazyServiceGetter(this, "gScreenManager",
> +XPCOMUtils.defineLazyServiceGetter(this, "gScreenManager",
I'm not sure what this is doing?

Comment 4

[[e-mail]]

(In reply to Philip Chee from comment #3)
> > -XPCOMUtils.defineLazyServiceGetter(this, "gScreenManager",
> > +XPCOMUtils.defineLazyServiceGetter(this, "gScreenManager",
> I'm not sure what this is doing?

DOS line ending...

Comment 5

[Ian Neal]

Comment on attachment 8395429 [details] [diff] [review]
With lots of bind and dispatch

>+++ b/suite/common/src/nsSessionStore.js

>       // give the tabbrowsers a chance to clear their histories first
>       var win = this._getMostRecentBrowserWindow();
>       if (win)
Are these two lines still needed? Is the comment still correct?

>-        win.setTimeout(function() { _this.saveState(true); }, 0);
>+        Services.tm.mainThread.dispatch(this.saveState.bind(this, true),
>+          Components.interfaces.nsIThread.DISPATCH_NORMAL);
>       else if (this._loadState == STATE_RUNNING)
>         this.saveState(true);
>       break;

r=me with that addressed.

Comment 6

[[e-mail]]

(In reply to Ian Neal from comment #5)
> (From update of attachment 8395429 [details] [diff] [review])
> >       // give the tabbrowsers a chance to clear their histories first
> >       var win = this._getMostRecentBrowserWindow();
> >       if (win)
> Are these two lines still needed? Is the comment still correct?
The comment is correct, but the temporary variable is no longer necessary; the code could be simplified to
if (this._getMostRecentBrowserWindow())
as we don't need to save the state if there is no browser window to save.

Comment 7

[Philip Chee]

Pushed to comm-central:
https://hg.mozilla.org/comm-central/rev/335dea6bd178
a=Callek for checkin to a CLOSED TREE

Neil does this need to be backported to 2.31? 2.30?

Comment 8

[[e-mail]]

I don't know, it's a bit of an edge case, but feel free to request approval.

Comment 9

[Philip Chee]

(In reply to [e-mail] from comment #8)
> I don't know, it's a bit of an edge case, but feel free to request approval.

In that case I say let it ride the trains.