Closed Bug 822093 Opened 13 years ago Closed 12 years ago

Intermittent browser_819510_perwindowpb.js | sessionstore state: 2 windows in data being writted to disk - Got 3, expected 2 | sessionstore state: 1 closed window in data being writted to disk - Got 0, expected 1

Categories

(Firefox :: Private Browsing, defect)

Product:
Firefox
Component:
Private Browsing
Version:
20 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 20

People

(Reporter: RyanVM, Assigned: andreshm)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 4 obsolete files)

Patch v5
6.74 KB, patch
jdm
: review+
Details | Diff | Splinter Review
Happening pretty frequently. https://tbpl.mozilla.org/php/getParsedLog.php?id=17990254&tree=Mozilla-Inbound Rev3 Fedora 12 mozilla-inbound opt test mochitest-browser-chrome on 2012-12-15 23:48:08 PST for push 3e4e600adc3b slave: talos-r3-fed-036 TEST-PASS | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_819510_perwindowpb.js | Window 2 is private TEST-PASS | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_819510_perwindowpb.js | Last window opened is the one selected TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_819510_perwindowpb.js | sessionstore state: 2 windows in data being writted to disk - Got 3, expected 2 Stack trace: JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 474 JS frame :: chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_819510_perwindowpb.js :: observe :: line 145 JS frame :: resource:///modules/sessionstore/SessionStore.jsm :: ssi_saveStateObject :: line 3685 JS frame :: resource:///modules/sessionstore/SessionStore.jsm :: ssi_saveState :: line 3672 JS frame :: resource:///modules/sessionstore/SessionStore.jsm :: ssi_onTimerCallback :: line 1138 JS frame :: resource:///modules/sessionstore/SessionStore.jsm :: ssi_observe :: line 579 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 TEST-PASS | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_819510_perwindowpb.js | Selected window is updated to match one of the saved windows TEST-PASS | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_819510_perwindowpb.js | Saved window is not private TEST-PASS | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_819510_perwindowpb.js | Saved window is not private TEST-PASS | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_819510_perwindowpb.js | Saved window is not private TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_819510_perwindowpb.js | sessionstore state: 1 closed window in data being writted to disk - Got 0, expected 1 Stack trace: JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 474 JS frame :: chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_819510_perwindowpb.js :: observe :: line 152 JS frame :: resource:///modules/sessionstore/SessionStore.jsm :: ssi_saveStateObject :: line 3685 JS frame :: resource:///modules/sessionstore/SessionStore.jsm :: ssi_saveState :: line 3672 JS frame :: resource:///modules/sessionstore/SessionStore.jsm :: ssi_onTimerCallback :: line 1138 JS frame :: resource:///modules/sessionstore/SessionStore.jsm :: ssi_observe :: line 579 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 INFO TEST-END | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_819510_perwindowpb.js | finished in 6485ms
Blocks: 819510
No longer depends on: 819510
Bug 819510 backed out on inbound, so these should stop once it propagates.
Assignee: nobody → andres
Attached patch Patch v1 (obsolete) — — Splinter Review
The problem is that we should wait for a window to close, in order to continue the test, saving state, etc. I added a executeSoon. The test is running fine locally. Pushed to try for a test run: https://tbpl.mozilla.org/?tree=Try&rev=79f383e56596
Attachment #692976 - Flags: review?(josh)
Comment on attachment 692976 [details] [diff] [review] Patch v1 Review of attachment 692976 [details] [diff] [review]: ----------------------------------------------------------------- If we need to wait for the window to close then maybe we should do exactly that. Waiting an arbitrary amount of time is not the solution here as the test would still be failing intermittently.
Attachment #692976 - Flags: review?(josh) → review-
Attached patch Patch v2 (obsolete) — — Splinter Review
Added observer to get the window close event. https://tbpl.mozilla.org/?tree=Try&rev=17c1e98fec0f
Attachment #692976 - Attachment is obsolete: true
Attachment #692982 - Flags: review?(ttaubert)
Comment on attachment 692982 [details] [diff] [review] Patch v2 Review of attachment 692982 [details] [diff] [review]: ----------------------------------------------------------------- Thank you, looks good. r=me with the fixes below. ::: browser/components/sessionstore/test/browser_819510_perwindowpb.js @@ +34,1 @@ > currentWindow.close(); We can use closeAllButPrimaryWindow() here. @@ +144,5 @@ > }); > } > > +function waitForWindowClose(aWin, aCallback) { > + Services.obs.addObserver(function observe(aSubject, aTopic, aData) { Need to compare aWin and aSubject here before continuing. @@ +161,5 @@ > + aSubject.QueryInterface(Ci.nsISupportsString); > + aCallback(JSON.parse(aSubject.data)); > + } > + }, "sessionstore-state-write", false); > + Services.prefs.setIntPref("browser.sessionstore.interval", 0); Let's decrease the interval once at the beginning and then use waitForSaveState().
Attachment #692982 - Flags: review?(ttaubert) → review+
Attached patch Patch v3 (obsolete) — — Splinter Review
(In reply to Tim Taubert [:ttaubert] from comment #22) > Comment on attachment 692982 [details] [diff] [review] > Patch v2 > > Review of attachment 692982 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thank you, looks good. > > r=me with the fixes below. > > ::: browser/components/sessionstore/test/browser_819510_perwindowpb.js > @@ +34,1 @@ > > currentWindow.close(); > > We can use closeAllButPrimaryWindow() here. > Done. > @@ +144,5 @@ > > }); > > } > > > > +function waitForWindowClose(aWin, aCallback) { > > + Services.obs.addObserver(function observe(aSubject, aTopic, aData) { > > Need to compare aWin and aSubject here before continuing. The subject for |xul-window-destroyed| is null. So, we can't compare it to the window. > @@ +161,5 @@ > > + aSubject.QueryInterface(Ci.nsISupportsString); > > + aCallback(JSON.parse(aSubject.data)); > > + } > > + }, "sessionstore-state-write", false); > > + Services.prefs.setIntPref("browser.sessionstore.interval", 0); > > Let's decrease the interval once at the beginning and then use > waitForSaveState(). |waitForSaveState| doesn't return the state for comparison (aSubject.data), also we need to force at a certain point to write the state, independently from opening/closing windows. I updated the method name for clarity.
Attachment #692982 - Attachment is obsolete: true
(In reply to Andres Hernandez [:andreshm] from comment #23) > > > +function waitForWindowClose(aWin, aCallback) { > > > + Services.obs.addObserver(function observe(aSubject, aTopic, aData) { > > > > Need to compare aWin and aSubject here before continuing. > > The subject for |xul-window-destroyed| is null. So, we can't compare it to > the window. Right, let's just use domwindowclosed then. > > @@ +161,5 @@ > > > + aSubject.QueryInterface(Ci.nsISupportsString); > > > + aCallback(JSON.parse(aSubject.data)); > > > + } > > > + }, "sessionstore-state-write", false); > > > + Services.prefs.setIntPref("browser.sessionstore.interval", 0); > > > > Let's decrease the interval once at the beginning and then use > > waitForSaveState(). > > |waitForSaveState| doesn't return the state for comparison (aSubject.data), > also we need to force at a certain point to write the state, independently > from opening/closing windows. I updated the method name for clarity. Ok, I don't mind keeping the existing code.
Status: NEW → ASSIGNED
Attached patch Patch v4 (obsolete) — — Splinter Review
Updated patch
Attachment #693393 - Attachment is obsolete: true
Failed on try :(
I think this was the run on bug 819510 comment 25, that was using the executeSoon patch. Just to be sure, I added a new run on try with the rebased full patch on bug 819510 comment 26. See: https://tbpl.mozilla.org/?tree=Try&rev=7030fd2cd0e3
Yes I did refer to bug 819510 comment 25, and looks like that didn't use executeSoon(): https://hg.mozilla.org/try/rev/17c1e98fec0f
Well, the latest push failed too. :( https://tbpl.mozilla.org/?tree=Try&rev=7030fd2cd0e3 I'll take a look again on this.
For future reference you can just run the mochitest-bc tests on try to conserve resources.
Sure, thanks for letting me know!
This bug was fixed with latest patch on Bug 819510 comment 31
Andes, the orange in comment 40 definitely occurred after the landing of bug 819510.
(In reply to Josh Matthews [:jdm] from comment #41) > Andes, the orange in comment 40 definitely occurred after the landing of bug > 819510. I'll take a look again at this :(, I don't have a clue why the window is not being registered as closed, since I added a check to continue only when |aWin.closed| is true. If you have any idea please let me know.
Attached patch Patch v5 — — Splinter Review
I updated the test to use the 'SSWindowClosing' event to get notified when the window is closing in the session store module. But, also, I'm double checking the browser state to continue only when the window is moved to |_closedWindows|. Since, this happens a moment after 'SSWindowClosing' event.
Attachment #693403 - Attachment is obsolete: true
Attachment #696410 - Flags: review?(josh)
Comment on attachment 696410 [details] [diff] [review] Patch v5 Review of attachment 696410 [details] [diff] [review]: ----------------------------------------------------------------- This seems reasonable to me.
Attachment #696410 - Flags: review?(josh) → review+
https://hg.mozilla.org/mozilla-central/rev/48d40bb9a11e
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Let's leave this open until we're sure it's fixed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
If you want the bug left open, please put [leave open] on the whiteboard when you push to inbound.
Seems like this is working fine! :)
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: