Open Bug 936065 Opened 11 years ago Updated 2 years ago

[Session Restore] setWindowState() will open and restore additional windows passed in aState

Categories

(Firefox :: Session Restore, defect)

defect

Tracking

()

REOPENED

People

(Reporter: smacleod, Unassigned)

References

Details

(Keywords: addon-compat)

Attachments

(1 file)

Since |setWindowState()| directly calls |restoreWindow()| with the entire state it is passed, it is possible to make |setWindowState()| open and restore multiple windows. We should remove this functionality, so that |setWindowState()| will only modify the state of the window it is passed.

There may be code which is abusing |setWindowState()| to open and restore multiple windows already, which should be investigated.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8341338 - Flags: review?(smacleod)
Comment on attachment 8341338 [details] [diff] [review]
0001-Bug-936065-Ignore-every-but-the-first-window-state-p.patch

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

LGTM!

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1508,4 @@
>      throw Components.Exception("Window is not tracked", Cr.NS_ERROR_INVALID_ARG);
>    },
>  
>    setWindowState: function ssi_setWindowState(aWindow, aState, aOverwrite) {

Maybe we should document this method while we're here changing the behavior?

@@ +1518,5 @@
> +      throw Components.Exception("Invalid state string: not JSON", Cr.NS_ERROR_INVALID_ARG);
> +    }
> +
> +    if (!winState.windows || !winState.windows[0]) {
> +      throw Components.Exception("Invalid window state passed", Cr.NS_ERROR_INVALID_ARG);

This appears to change the behavior of setWindowState to throw an exception if there is no window state, where before it would have just not restored.

I think this new behavior makes more sense, but could cause breakages with callers. We should keep an eye out for this as well as breakages from the change to only restore a single window.
Attachment #8341338 - Flags: review?(smacleod) → review+
OS: Mac OS X → All
Hardware: x86 → All
(In reply to Steven MacLeod [:smacleod] from comment #2)
> >      throw Components.Exception("Window is not tracked", Cr.NS_ERROR_INVALID_ARG);
> >    },
> >  
> >    setWindowState: function ssi_setWindowState(aWindow, aState, aOverwrite) {
> 
> Maybe we should document this method while we're here changing the behavior?

Sure.

> This appears to change the behavior of setWindowState to throw an exception
> if there is no window state, where before it would have just not restored.
> 
> I think this new behavior makes more sense, but could cause breakages with
> callers. We should keep an eye out for this as well as breakages from the
> change to only restore a single window.


I don't really expect any big breakage but we should definitely watch out for any incompatibility that might result from this change.
Keywords: addon-compat
https://hg.mozilla.org/mozilla-central/rev/7b156069de68
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
This was backed out from 28/29 in bug 950320.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 28 → ---
Thank you, Ryan. This got lost somewhere...
Assignee: ttaubert → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: