The default bug view has changed. See this FAQ.

SessionStore.onLoad missed for a window that opened before the first window in the session fired browser-delayed-startup-finished

VERIFIED FIXED in Firefox 25

Status

()

Firefox
Session Restore
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: tabmix.onemen, Assigned: smacleod)

Tracking

({verifyme})

Trunk
Firefox 27
verifyme
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox25+ fixed, firefox26+ verified, firefox27 verified)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

4 years ago
Since bug 881049 and bug 898308 SessionStore.init run only after gBrowserInit._delayedStartup finished for the first window in the session.

SessionStore.onLoad for new windows after the first one relay on observing "domwindowopened"

If more windows are opened before SessionStore.init runs, all windows except the first one will be unknown to SessionStore.
(Reporter)

Updated

4 years ago
Blocks: 911241
Steven, do you think you can take a look at this?
Flags: needinfo?(smacleod)
(Assignee)

Comment 2

4 years ago
(In reply to Jared Wein [:jaws] from comment #1)
> Steven, do you think you can take a look at this?

Sure.
Assignee: nobody → smacleod
Status: NEW → ASSIGNED
Flags: needinfo?(smacleod)
What regressed this exactly? Sounds like maybe something worth tracking for whatever release we first regressed this in.
Flags: needinfo?(smacleod)
(Assignee)

Comment 4

4 years ago
I believe I was able to exercise this bug a bit. By opening windows rapidly while firefox is opening, using the OSX hotkey for new window, I'm able to have Session Store lose one of the windows. I tested this on a build before the patch mentioned by onemen ( http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013/07/2013-07-27-03-02-06-mozilla-central/ ), and windows do not appear to be lost there.

Session Store does fail to restore anything, even on the previous build which does not lose windows, so I believe there are other bugs present when windows are opened rapidly at startup.

Gavin, it appears the regression is was introduced by the patch in Bug 898308.
Flags: needinfo?(smacleod)
status-firefox25: --- → affected
status-firefox26: --- → affected
tracking-firefox25: --- → +
tracking-firefox26: --- → +
Blocks: 898308
(Assignee)

Updated

4 years ago
Depends on: 922178
(Assignee)

Updated

4 years ago
Blocks: 918024

Comment 5

4 years ago
I'm guessing that since bug 898308 causes add-on compat changes, backing out is not an option.

I'm wondering if this should track at all, since the STR appear to be:

1) Open Firefox
2) Hit cmd-N before session restore occurs
3) Close Firefox
4) Open Firefox, notice that the window from #2 is missing

Very edge casey
Flags: needinfo?(gavin.sharp)

Comment 6

4 years ago
(that needinfo was a "do you agree, or do you think we should still take a forward fix" :)
I think we don't fully understand how likely this is to hit in practice. The STR you describe are indeed edge cases, but it's possible that add-on interactions would also trigger this.

Steven, can you prioritize work on this bug and figure out what the "forward fix" would look like?
Flags: needinfo?(gavin.sharp) → needinfo?(smacleod)
(Assignee)

Comment 8

4 years ago
Created attachment 814990 [details] [diff] [review]
Patch - Initialize SessionStore earlier to catch windows opened immediately after startup

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7)
> I think we don't fully understand how likely this is to hit in practice. The
> STR you describe are indeed edge cases, but it's possible that add-on
> interactions would also trigger this.

This is also triggered by the case in Bug 911241. The steps I listed are just the way I was able to reproduce it.

It should also be noted that this causes your session restore to fail as well. When that extra window sneaks in, it has |SessionStoreInternal.onLoad()| called on it before the first window, and the restore fails.

With that in mind, I think we should probably still go with a "forward fix".

> Steven, can you prioritize work on this bug and figure out what the "forward
> fix" would look like?

I'm posting a patch which does a couple things to solve this: 
a) Initializes session store earlier, before any domwindowopened events have fired.
b) Moves the |onLoad| call on the first window out of init, and into the domwindowopened observer code path.
c) Ensures all windows wait for the session file to be read before calling onLoad, so that we don't trigger the synchronous session file read fallback path.
Attachment #814990 - Flags: review?(gavin.sharp)
Attachment #814990 - Flags: review?(dteller)
Flags: needinfo?(smacleod)
(Assignee)

Comment 9

4 years ago
Comment on attachment 814990 [details] [diff] [review]
Patch - Initialize SessionStore earlier to catch windows opened immediately after startup

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ -703,5 @@
>          this._loadState == STATE_QUITTING)
>        return;
>  
> -    // assign it a unique identifier (timestamp)
> -    aWindow.__SSi = "window" + Date.now();

Just wanted to point out, I changed this here since I was able to actually observe collisions in the identifiers generated. Probably due to making the windows wait on the promise, and having onLoad called in rapid succession now.
Hmm, I definitely see the value in simplifying the initialization process this way, but I'm a bit wary of uplifting that to beta, with so little time for "baking" - seems too likely to reveal all sorts of potential timing problems.

Do we have any other options, or is this just inevitable given our asyncification work?
(Assignee)

Comment 11

4 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #10)
> Hmm, I definitely see the value in simplifying the initialization process
> this way, but I'm a bit wary of uplifting that to beta, with so little time
> for "baking" - seems too likely to reveal all sorts of potential timing
> problems.
> 
> Do we have any other options, or is this just inevitable given our
> asyncification work?

My first attempt would iterate the open windows after initialization, to catch any that were missed. The issue with that approach though, is I can't tell if a window has fired "load" yet, so I don't know if I should call onLoad immediately, or attach an event.

Another option we could possibly take is backout the patch from Bug 904529 as well, which would require less changes to the onOpen handler.

Or, we could just go back to calling |SessionStore.init()| for every window in |_delayedStartup()|, backing out Bug 898308. This just happened to be catching the windows we missed "domwindowopened" for, and calling |onLoad|.

I really think it's best to move the |SessionStore.init()| call to where my patch puts it though, since the model of how we discover windows becomes much simpler. All windows are tracked by listening for their "domwindowopened", instead of special casing the first window.

The current patch definitely seems like the cleanest solution to me, and makes the initialization much easier to reason about, cutting things down to a single window initialization path. I'd say it probably removes timing problems if anything, but I definitely understand your wariness.
(Assignee)

Comment 12

4 years ago
Try: https://tbpl.mozilla.org/?tree=Try&rev=8344182ab138
(In reply to Steven MacLeod [:smacleod] from comment #11)
> My first attempt would iterate the open windows after initialization, to
> catch any that were missed. The issue with that approach though, is I can't
> tell if a window has fired "load" yet, so I don't know if I should call
> onLoad immediately, or attach an event.

You could check document.readyState, but...

> Another option we could possibly take is backout the patch from Bug 904529
> as well, which would require less changes to the onOpen handler.

> Or, we could just go back to calling |SessionStore.init()| for every window
> in |_delayedStartup()|, backing out Bug 898308. This just happened to be
> catching the windows we missed "domwindowopened" for, and calling |onLoad|.

Either of these options seem potentially viable. What are the risks?

> The current patch definitely seems like the cleanest solution to me, and
> makes the initialization much easier to reason about, cutting things down to
> a single window initialization path. I'd say it probably removes timing
> problems if anything, but I definitely understand your wariness.

I agree; I have no concerns about landing it on trunk. But landing it in one of the last betas is a different story, hence the pushback.
Comment on attachment 814990 [details] [diff] [review]
Patch - Initialize SessionStore earlier to catch windows opened immediately after startup

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

Unfortunately, I haven't followed the offending patch(es?), so I'm not sure I grasp all implications (hence my f+ instead of a r+).
However, I like this patch. I believe that it makes things more logical.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +93,5 @@
>    "@mozilla.org/browser/sessionstartup;1", "nsISessionStartup");
>  XPCOMUtils.defineLazyServiceGetter(this, "gScreenManager",
>    "@mozilla.org/gfx/screenmanager;1", "nsIScreenManager");
> +XPCOMUtils.defineLazyServiceGetter(this, "uuidGenerator",
> +  "@mozilla.org/uuid-generator;1", "nsIUUIDGenerator");

Good idea.

@@ +366,5 @@
>  
>    /**
>     * Initialize the sessionstore service.
>     */
> +  init: function () {

That looks saner.
Attachment #814990 - Flags: review?(dteller) → feedback+
Comment on attachment 814990 [details] [diff] [review]
Patch - Initialize SessionStore earlier to catch windows opened immediately after startup

>+          if (!this._sessionInitialized){

add a space between ) and {
Attachment #814990 - Flags: review+
(Assignee)

Comment 16

4 years ago
Created attachment 815881 [details] [diff] [review]
Beta Patch - Back out changeset 1e1f3cd07479 (Bug 898308)

The code in my new patch is a bit too complex to uplift to beta, so we should probably just backout the single offending change.

This patch backs out Bug 898308 on beta. There were no conflicts during the backout, so it should be pretty straightforward.
Attachment #815881 - Flags: review?(gavin.sharp)
Attachment #815881 - Flags: review?(dao)
(Assignee)

Comment 17

4 years ago
Created attachment 815887 [details] [diff] [review]
Aurora Patch 1 - Back out changeset 68f187b1e3e5 (Bug 904529)

Aurora has an additional patch landed which relies on the changed code from of Bug 898308. This is the first backout patch.

This patch backs out Bug 904529 on aurora. There were a couple conflicts during the backout  which I manually fixed, but they were mostly just mechanical.
Attachment #815887 - Flags: review?(dteller)
(Assignee)

Comment 18

4 years ago
Created attachment 815891 [details] [diff] [review]
Aurora Patch 2 - Back out changeset 1e1f3cd07479 (Bug 898308)

This patch backs out Bug 898308 on aurora. There were a couple conflicts during the backout which I manually fixed.

Conflicts:
	browser/base/content/browser.js
	browser/components/sessionstore/src/SessionStore.jsm

Some of the conflicts were mechanical, but some of the e10s specific stuff had been added since the change. It would be good to get some scrutiny over the conflicting files during review.
Attachment #815891 - Flags: review?(wmccloskey)
Attachment #815891 - Flags: review?(dao)
(Assignee)

Comment 19

4 years ago
Try Push for Beta Backouts: https://tbpl.mozilla.org/?tree=Try&rev=7ff1084dc742
Try Push for Aurora Backouts: https://tbpl.mozilla.org/?tree=Try&rev=eb739cb1280c

Updated

4 years ago
Attachment #815881 - Flags: review?(dao) → review+
Comment on attachment 815891 [details] [diff] [review]
Aurora Patch 2 - Back out changeset 1e1f3cd07479 (Bug 898308)

>--- a/browser/components/sessionstore/src/SessionStore.jsm
>+++ b/browser/components/sessionstore/src/SessionStore.jsm

I don't understand why you're moving the _disabledForMultiProcess code from initService to init.
(Assignee)

Comment 21

4 years ago
(In reply to Dão Gottwald [:dao] from comment #20)
> Comment on attachment 815891 [details] [diff] [review]
> Aurora Patch 2 - Back out changeset 1e1f3cd07479 (Bug 898308)
> 
> >--- a/browser/components/sessionstore/src/SessionStore.jsm
> >+++ b/browser/components/sessionstore/src/SessionStore.jsm
> 
> I don't understand why you're moving the _disabledForMultiProcess code from
> initService to init.

I'm not, the position of init in the file changes in the patch.
Comment on attachment 815881 [details] [diff] [review]
Beta Patch - Back out changeset 1e1f3cd07479 (Bug 898308)

>diff --git a/browser/components/sessionstore/nsISessionStore.idl b/browser/components/sessionstore/nsISessionStore.idl

>-[scriptable, uuid(700756cc-f5c7-11e2-b842-59d9dc830245)]
>+[scriptable, uuid(092fa0cc-e99b-11e2-a2a3-a25b4f45d8e2)]
> interface nsISessionStore : nsISupports
> {
>   /**
>+   * Initialize the service
>+   */
>+  jsval init(in nsIDOMWindow aWindow);

The need for the IDL change is unfortunate. I think the likelihood of this being used from native code is close to zero, though, so we should be OK. To avoid possible compat issues we could keep the same IID and add the method at the end of the IDL, though.
Attachment #815881 - Flags: review?(gavin.sharp) → review+
Comment on attachment 814990 [details] [diff] [review]
Patch - Initialize SessionStore earlier to catch windows opened immediately after startup

This looks good overall for trunk, but I had a few concerns about the changes to the window ID that Steven said he was going to rework (we don't actually need GUIDs, we can just use a simple counter).
Attachment #814990 - Flags: review?(gavin.sharp) → feedback+
(In reply to Steven MacLeod [:smacleod] from comment #18)
> Created attachment 815891 [details] [diff] [review]
> Aurora Patch 2 - Back out changeset 1e1f3cd07479 (Bug 898308)
> 
> This patch backs out Bug 898308 on aurora. There were a couple conflicts
> during the backout which I manually fixed.

Bug 898308 is marked as blocking bug 898732 and bug 910167, which landed on what is now Aurora. Do those need to be reverted too?
(Assignee)

Comment 25

4 years ago
Created attachment 815991 [details] [diff] [review]
Beta Patch - Back out changeset 1e1f3cd07479 (Bug 898308) v2

Updated the patches IDL change as suggested by Gavin.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 898308

User impact if declined: Session Store may lose windows opened rapidly, which will result in their tabs not being saved, along with other problems such as not being able to close tabs within that window (Bug 911241).

Testing completed (on m-c, etc.): 
Try Push: https://tbpl.mozilla.org/?tree=Try&rev=52b9eff0540a

Risk to taking this patch (and alternatives if risky): Should be pretty low risk, as it is a straightforward backout.

String or IDL/UUID changes made by this patch: nsISessionStore.idl has an additional method added to the end, that was removed in the patch being backed out. The uuid is remaining unchanged to avoid compat issues, as suggested by Gavin. "the likelihood of this being used from native code is close to zero"
Attachment #815881 - Attachment is obsolete: true
Attachment #815991 - Flags: approval-mozilla-beta?
(Assignee)

Comment 26

4 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #24)
> (In reply to Steven MacLeod [:smacleod] from comment #18)
> > Created attachment 815891 [details] [diff] [review]
> > Aurora Patch 2 - Back out changeset 1e1f3cd07479 (Bug 898308)
> > 
> > This patch backs out Bug 898308 on aurora. There were a couple conflicts
> > during the backout which I manually fixed.
> 
> Bug 898308 is marked as blocking bug 898732 and bug 910167, which landed on
> what is now Aurora. Do those need to be reverted too?

I believe this should be fine, as those changes just switch calls to use the JSM, and I left |defineLazyModuleGetter()| call in. This is assuming it is okay to mix use of the JSM directly and the IDL, is this a correct assumption?
Comment on attachment 815891 [details] [diff] [review]
Aurora Patch 2 - Back out changeset 1e1f3cd07479 (Bug 898308)

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

I don't really care what happens with e10s on aurora.
Attachment #815891 - Flags: review?(wmccloskey) → review+
(In reply to Steven MacLeod [:smacleod] from comment #26)
> I believe this should be fine, as those changes just switch calls to use the
> JSM, and I left |defineLazyModuleGetter()| call in. This is assuming it is
> okay to mix use of the JSM directly and the IDL, is this a correct
> assumption?

Given that it looks to just be a straight mapping (browser/components/sessionstore/src/nsSessionStore.js), sounds fine!
Comment on attachment 815887 [details] [diff] [review]
Aurora Patch 1 - Back out changeset 68f187b1e3e5 (Bug 904529)

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

r=me, but with a limited confidence in my ability to review a backout
Attachment #815887 - Flags: review?(dteller) → review+

Updated

4 years ago
Attachment #815991 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/6006e21fe7c8
status-firefox25: affected → fixed
(Assignee)

Comment 31

4 years ago
Created attachment 817905 [details] [diff] [review]
Patch - Initialize SessionStore earlier to catch windows opened immediately after startup v2

This updated patch no longer changes the window ID generation, and is based on top of the patch in Bug 925771. It's important that this patch has the ID generation changes, since it is much more likely to cause collisions (Now that new windows will all wait for the session initialization, if any windows are opened before this happens, they will all have there IDs generated close together in time).

I've also changed the onLoad handler to not make new windows wait on |gSessionStartup.onceInitialized| if we know it is already resolved. Waiting on this promise causes |SessionStoreInternal.onLoad()| to be executed one tick later, which can cause a number of problems if initialization is finished. This seems to clear up all of the unit tests failures.
Attachment #814990 - Attachment is obsolete: true
Attachment #817905 - Flags: review?(ttaubert)
(Assignee)

Comment 32

4 years ago
Try push for attachment 817905 [details] [diff] [review] https://tbpl.mozilla.org/?tree=Try&rev=7c8c52297540
Comment on attachment 817905 [details] [diff] [review]
Patch - Initialize SessionStore earlier to catch windows opened immediately after startup v2

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

Great.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +860,5 @@
>        aWindow.removeEventListener("load", onload);
> +
> +      if (aWindow.closed) {
> +        return;
> +      }

Do we really need that check here? Seems like at this point when notified this should never be true.

@@ +881,5 @@
> +
> +        if (this._sessionInitialized) {
> +          this.onLoad(aWindow);
> +        } else {
> +          let initialState = this.initSession();

I know that _sessionInitialized is being set by initSession() but this function would be a little more obvious if we could do it here? That would eliminate the need to check all of the stuff it's calling to ensure that it's working correctly.
Attachment #817905 - Flags: review?(ttaubert) → review+
Comment on attachment 815891 [details] [diff] [review]
Aurora Patch 2 - Back out changeset 1e1f3cd07479 (Bug 898308)

(In reply to Steven MacLeod [:smacleod] from comment #21)
> (In reply to Dão Gottwald [:dao] from comment #20)
> > Comment on attachment 815891 [details] [diff] [review]
> > Aurora Patch 2 - Back out changeset 1e1f3cd07479 (Bug 898308)
> > 
> > >--- a/browser/components/sessionstore/src/SessionStore.jsm
> > >+++ b/browser/components/sessionstore/src/SessionStore.jsm
> > 
> > I don't understand why you're moving the _disabledForMultiProcess code from
> > initService to init.
> 
> I'm not, the position of init in the file changes in the patch.

Before https://hg.mozilla.org/mozilla-central/rev/1e1f3cd07479, the _disabledForMultiProcess stuff was in initService.
Attachment #815891 - Flags: review?(dao) → review-
(Assignee)

Comment 35

4 years ago
(In reply to Dão Gottwald [:dao] from comment #34)
> Comment on attachment 815891 [details] [diff] [review]
> Aurora Patch 2 - Back out changeset 1e1f3cd07479 (Bug 898308)
> 
> (In reply to Steven MacLeod [:smacleod] from comment #21)
> > (In reply to Dão Gottwald [:dao] from comment #20)
> > > Comment on attachment 815891 [details] [diff] [review]
> > > Aurora Patch 2 - Back out changeset 1e1f3cd07479 (Bug 898308)
> > > 
> > > >--- a/browser/components/sessionstore/src/SessionStore.jsm
> > > >+++ b/browser/components/sessionstore/src/SessionStore.jsm
> > > 
> > > I don't understand why you're moving the _disabledForMultiProcess code from
> > > initService to init.
> > 
> > I'm not, the position of init in the file changes in the patch.
> 
> Before https://hg.mozilla.org/mozilla-central/rev/1e1f3cd07479, the
> _disabledForMultiProcess stuff was in initService.

Yes, but there have been patches since: https://hg.mozilla.org/mozilla-central/rev/eb7bf75e0a28 which make it so this code really should be init. So, the code I'm "moving" is code that actually was introduced directly into init, not initService.

Sorry, I should have been much more explicit about this.
(In reply to Steven MacLeod [:smacleod] from comment #35)
> Yes, but there have been patches since:
> https://hg.mozilla.org/mozilla-central/rev/eb7bf75e0a28 which make it so
> this code really should be init. So, the code I'm "moving" is code that
> actually was introduced directly into init, not initService.
> 
> Sorry, I should have been much more explicit about this.

Also, as I said, this code is pretty much irrelevant on Aurora. We don't expect anyone to be testing e10s there.
(Assignee)

Comment 37

4 years ago
Created attachment 818509 [details] [diff] [review]
Patch - Initialize SessionStore earlier to catch windows opened immediately after startup v3

(In reply to Tim Taubert [:ttaubert] (limited availability, work week) from comment #33)
> Do we really need that check here? Seems like at this point when notified
> this should never be true.

Yeah, you're correct, I think that was blindly being over cautious.

> I know that _sessionInitialized is being set by initSession() but this
> function would be a little more obvious if we could do it here? That would
> eliminate the need to check all of the stuff it's calling to ensure that
> it's working correctly.

Definitely, much more readable with it pulled out.
Attachment #817905 - Attachment is obsolete: true
(Assignee)

Comment 38

4 years ago
attachment 818509 [details] [diff] [review] should be good to land on fx-team
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Whiteboard: [leave open]
(Assignee)

Comment 39

4 years ago
Gavin, (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #10)
> Hmm, I definitely see the value in simplifying the initialization process
> this way, but I'm a bit wary of uplifting that to beta, with so little time
> for "baking" - seems too likely to reveal all sorts of potential timing
> problems.

After talking with Tim, we think it might just be best to uplift the new patch to aurora, instead of doing the backout there. This would also require uplifting the patch in Bug 925771 as well.

Are you concerned with an uplift to aurora as well, or would you be fine with that?
Flags: needinfo?(gavin.sharp)
Sounds OK.
Flags: needinfo?(gavin.sharp)
(Assignee)

Updated

4 years ago
Whiteboard: [leave open]
https://hg.mozilla.org/integration/fx-team/rev/1374158dab4b
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 42

4 years ago
Created attachment 818621 [details] [diff] [review]
Aurora Patch - Uplift to Aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 898308


User impact if declined: Possible loss of Session data for windows opened quickly during startup.


Testing completed (on m-c, etc.): 
Currently on fx-team: https://tbpl.mozilla.org/?tree=Fx-Team&rev=62732da6ae3d
m-c try push: https://tbpl.mozilla.org/?tree=Try&rev=461d27cc7fe3
aurora try push: https://tbpl.mozilla.org/?tree=Try&rev=494b33a59cc9
Manually tested on both m-c and aurora to verify it fixes regression and tests pass.


Risk to taking this patch (and alternatives if risky): This patch changes the timing of session store initialization, which could interact with another component in an unforeseen way. Since Session Store was making some incorrect assumptions about timing, and this patch makes those assumptions correct, we're probably in a much better state.

The alternative would be to backout the offending change, along with another change introduced after which now relies on the code. There has also been additional commits in the mean time, which can be adapted to the backout, but may interact in ways I have not anticipated.


String or IDL/UUID changes made by this patch: None

This patch should only be uplifted with the patch in Bug 925771 - It will cause the ID collisions Bug 925771 fixes to happen much more frequently.
Attachment #815887 - Attachment is obsolete: true
Attachment #815891 - Attachment is obsolete: true
Attachment #818621 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/1374158dab4b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
(Reporter)

Comment 44

4 years ago
restoreWindow check if window initialized and call onLoad if not.

>    // initialize window if necessary
>    if (aWindow && (!aWindow.__SSi || !this._windows[aWindow.__SSi]))
>      this.onLoad(aWindow);

theoretically, setWindowState or setBrowserState can call restoreWindow before initialization complete

don't you think you need to use gSessionStartup.onceInitialized here?
Attachment #818621 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/99604e27f4a4
status-firefox26: affected → fixed
status-firefox27: --- → fixed
Depends on: 928630
Depends on: 929097

Updated

4 years ago
Keywords: verifyme

Updated

4 years ago
Depends on: 929849

Comment 46

3 years ago
Verified as fixed on Firefox 26b1 - 20131028225529 - on Mac OS X 10.9.


I tried to also verify it on Windows 7 and Ubuntu 13.04, but Ctrl+N opens a new window from the last focused app until the first Firefox window gets opened. On Mac OS X, I could reproduce (then verify) the bug by opening a window between the moment the Firefox icon appeared in the dock and when the window with the session opened.
status-firefox26: fixed → verified
QA Contact: ioana.budnar
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:27.0) Gecko/20100101 Firefox/27.0
Verified as fixed on latest Aurora (20131106004000), both windows are restored.
Status: RESOLVED → VERIFIED
status-firefox27: fixed → verified
You need to log in before you can comment on or make changes to this bug.