Closed
Bug 462986
Opened 16 years ago
Closed 16 years ago
Fix private browsing tests to pass on tinderbox (session store test)
Categories
(Firefox :: Private Browsing, defect)
Firefox
Private Browsing
Tracking
()
RESOLVED
FIXED
Firefox 3.1b2
People
(Reporter: sdwilsh, Assigned: ehsan.akhgari)
References
Details
(Keywords: fixed1.9.1)
Attachments
(3 files, 1 obsolete file)
1.75 KB,
patch
|
zeniko
:
review+
|
Details | Diff | Splinter Review |
1.94 KB,
patch
|
Details | Diff | Splinter Review | |
2.27 KB,
patch
|
zeniko
:
review+
|
Details | Diff | Splinter Review |
After landing bug 248970, one test failed on tinderbox. I made it a todo, with a comment referencing this bug. We should understand what the failure is before we release beta 2 in my opinion, but I do not think it was enough to backout the whole feature. The test in question is browser_248970_a.js in sessionstore. So far it has failed on Linux and windows. Still waiting on OS X
Flags: blocking-firefox3.1?
Reporter | ||
Comment 1•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/26e3e990a2f3
Assignee | ||
Comment 2•16 years ago
|
||
OK, so this is basically a timing issue. If the test is executed quickly enough, then the timestamps will be the same after entering the private browsing mode, even though saveState *has* been called. A delay as low as 50ms should be enough here, to ensure that the timestamps *will* be different if the file is written on entry, and also to make the test take as little time as possible. (I'm sorry for how the patch is, couldn't make diff -w work; but anyway the code inside the timer has not changed -- I only removed Shawn's comment.)
Attachment #346213 -
Flags: review?(zeniko)
Updated•16 years ago
|
Attachment #346213 -
Flags: review?(zeniko) → review+
Assignee | ||
Comment 3•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/fbae114d6133
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 4•16 years ago
|
||
I've backed this out due to the test failures on Linux
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 5•16 years ago
|
||
Simon: do you think we need a larger timeout here? I really think that this is a timing issue, but I don't want to set a large timeout value to make tests slower than needed (see bug 463021).
Comment 6•16 years ago
|
||
If the writes are too quick in succession, we might have to use a different strategy: Just delete sessionstore.js and verify that it gets recreated (which should equal an updated lastModifiedDate but apparently doesn't).
Assignee | ||
Comment 7•16 years ago
|
||
(In reply to comment #6) > If the writes are too quick in succession, we might have to use a different > strategy: Just delete sessionstore.js and verify that it gets recreated (which > should equal an updated lastModifiedDate but apparently doesn't). This is a great idea. Here's the patch which does this. No more flaky timeouts! :-)
Attachment #346213 -
Attachment is obsolete: true
Attachment #346322 -
Flags: review?(zeniko)
Comment 8•16 years ago
|
||
Comment on attachment 346322 [details] [diff] [review] Second try Sure, just please be somewhat more specific with the variable names: What "path" and what "file"? r+=me with that fixed.
Attachment #346322 -
Flags: review?(zeniko) → review+
Assignee | ||
Comment 9•16 years ago
|
||
Assignee | ||
Comment 10•16 years ago
|
||
I may not be around long enough tonight to check this in myself...
Keywords: checkin-needed
Assignee | ||
Comment 11•16 years ago
|
||
We decided to leave this till after Beta 2's freeze.
Keywords: checkin-needed
Target Milestone: Firefox 3.1b2 → Firefox 3.1
Assignee | ||
Comment 12•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/d6b4f82c604b
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: Firefox 3.1 → Firefox 3.1b2
Assignee | ||
Comment 13•16 years ago
|
||
This is still causing some oranges. Example: <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1226216246.1226219612.30745.gz> The nature of this failure is exactly like the previous one described in comment 6. I'll attach a patch with the same fix as the previous patch landed for the failing test.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 14•16 years ago
|
||
Attachment #347144 -
Flags: review?(zeniko)
Assignee | ||
Updated•16 years ago
|
Attachment #347144 -
Attachment is patch: true
Attachment #347144 -
Attachment mime type: application/octet-stream → text/plain
Comment 15•16 years ago
|
||
Comment on attachment 347144 [details] [diff] [review] Third try + // set the interval to 0 This is pretty much obvious from the code. ;-) If anything, you rather want to describe the desired effect(s) this has.
Attachment #347144 -
Flags: review?(zeniko) → review+
Assignee | ||
Comment 16•16 years ago
|
||
(In reply to comment #15) > (From update of attachment 347144 [details] [diff] [review]) > + // set the interval to 0 > > This is pretty much obvious from the code. ;-) > If anything, you rather want to describe the desired effect(s) this has. Fixed on check-in. http://hg.mozilla.org/mozilla-central/rev/8b146fe55de9
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Updated•16 years ago
|
Keywords: fixed1.9.1
Assignee | ||
Comment 17•16 years ago
|
||
Mass moving of all Firefox::General private browsing bugs to Firefox::Private Browsing.
Component: General → Private Browsing
Assignee | ||
Updated•16 years ago
|
QA Contact: general → private.browsing
You need to log in
before you can comment on or make changes to this bug.
Description
•