Closed Bug 539660 Opened 14 years ago Closed 13 years ago

Remove sessionstore.interval and sessionstore.resume_from_crash default values from nsSessionStore.js

Categories

(Firefox :: Session Restore, defect)

defect
Not set
trivial

Tracking

()

VERIFIED FIXED
Firefox 5

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
Bug 535408 comment 4 doesn't make sense; we couldn't remove it from firefox.js, as getIntPref would throw.
Attachment #421614 - Flags: review?(paul)
Comment on attachment 421614 [details] [diff] [review]
patch

(In reply to comment #0)
> Bug 535408 comment 4 doesn't make sense; we couldn't remove it from firefox.js,
> as getIntPref would throw.

Ok fine. Bad comment there. But what's the value in moving all of these to lazy getters? It seems to me this would create more overhead.

_prefBranch is used later in init(), so there's pretty much no value in moving that.

The prefs - ok. We don't need those right away so that move is probably ok.
Defining JS getters shouldn't create any measurable overhead. Pretty much anything not directly used in init() should be a lazy getter.

Defining _interval and _resume_from_crash as lazy getters but not _prefBranch would make them depend on init() being called first, whereas currently they can be accessed before init(). I suppose that shouldn't be a problem in practice, but I'd like to avoid the not-immediately-obvious dependency anyway.
Paul: ping?
Comment on attachment 421614 [details] [diff] [review]
patch

Sorry, slipped off my radar. r=zpao
Attachment #421614 - Flags: review?(paul) → review+
http://hg.mozilla.org/mozilla-central/rev/6f48ddc83ea7
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
I backed this out attempting to fix a persistent orange on all platforms.  If it doesn't fix the tree I will reland it.

The failures look like this on Windows

s: win32-slave13
TEST-UNEXPECTED-FAIL | e:\builds\moz2_slave\mozilla-central-win32-debug-unittest-xpcshell\build\xpcshell\tests\test_cookies\unit\test_bug468700.js | test failed (with xpcshell return code: -1073741819), see following log:
PROCESS-CRASH | e:\builds\moz2_slave\mozilla-central-win32-debug-unittest-xpcshell\build\xpcshell\tests\test_cookies\unit\test_bug468700.js | application crashed (minidump found)
Thread 0 (crashed)

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264252531.1264254573.11593.gz

Mac & Linux:
s: moz2-linux-slave24
TEST-UNEXPECTED-FAIL | /builds/slave/mozilla-central-linux-debug-unittest-xpcshell/build/xpcshell/tests/test_cookies/unit/test_bug468700.js | test failed (with xpcshell return code: -6), see following log:
PROCESS-CRASH | /builds/slave/mozilla-central-linux-debug-unittest-xpcshell/build/xpcshell/tests/test_cookies/unit/test_bug468700.js | application crashed (minidump found)
Thread 0 (crashed)

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264255005.1264256901.6443.gz

Pushed http://hg.mozilla.org/mozilla-central/rev/35813f2991a8
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Just ran this locally to get a JS stack:

0 [native frame]
1 sss_openWindowWithState(aState = [object Object]) ["file:///Users/pao/mozilla/mozilla-central-pbranch/obj-ff-debug/dist/bin/components/nsSessionStore.js":2638]
    ID = undefined
    window = undefined
    features = "chrome,dialog=no,all"
    argString = 
    winState = [object Object]
    this = [object Object]
2 sss_setBrowserState(aState = "{"windows":[{"tabs":[{"entries":[{"url":"about:privatebrowsing"}]}],"_closedTabs":[]}]}") ["file:///Users/pao/mozilla/mozilla-central-pbranch/obj-ff-debug/dist/bin/components/nsSessionStore.js":868]
    window = null
    state = [object Object]
    this = [object Object]
3 [native frame]
4 PBS__onAfterPrivateBrowsingModeChange() ["file:///Users/pao/mozilla/mozilla-central-pbranch/obj-ff-debug/dist/bin/components/nsPrivateBrowsingService.js":244]
    this = [object Object]
5 PBS_set_privateBrowsingEnabled(val = true) ["file:///Users/pao/mozilla/mozilla-central-pbranch/obj-ff-debug/dist/bin/components/nsPrivateBrowsingService.js":483]
    this = [object Object]
6 [native frame]
7 run_test() ["/Users/pao/mozilla/mozilla-central-pbranch/obj-ff-debug/_tests/xpcshell/test_cookies/unit/test_bug468700.js":34]
    uri = [xpconnect wrapped nsIURI @ 0x4ecec90 (native @ 0x4ece984)]
    spec = "http://foo.bar/baz"
    pb = [xpconnect wrapped nsIPrivateBrowsingService @ 0x4ec2a20 (native @ 0x8a64b0)]
    prefs = [xpconnect wrapped nsIPrefBranch @ 0x4ec2680 (native @ 0x8323e0)]
    ios = [xpconnect wrapped (nsISupports, nsIIOService2, nsIIOService) @ 0x4ebb880 (native @ 0x835530)]
    cm = [xpconnect wrapped (nsISupports, nsICookieService, nsICookieManager2) @ 0x4ec2100 (native @ 0x4ec1780)]
    cs = [xpconnect wrapped (nsISupports, nsICookieService, nsICookieManager2) @ 0x4ec2100 (native @ 0x4ec1780)]
    this = [object BackstagePass @ 0x4eb78f0 (native @ 0x839194)]
8 _execute_test() ["/Users/pao/mozilla/mozilla-central-pbranch/testing/xpcshell/head.js":130]
    truePassedChecks = undefined
    func = undefined
    this = [object BackstagePass @ 0x4eb78f0 (native @ 0x839194)]
9 <TOP LEVEL> ["typein":1]
    this = [object BackstagePass @ 0x4eb78f0 (native @ 0x839194)]
The changes look innocuous enough. The fact that this caused a crash is scary.
http://hg.mozilla.org/projects/cedar/rev/89ff348a5fe3
Whiteboard: fixed-in-cedar
Target Milestone: Firefox 3.7a1 → ---
http://hg.mozilla.org/mozilla-central/rev/89ff348a5fe3
Status: REOPENED → RESOLVED
Closed: 14 years ago13 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → Firefox4.2
Blocks: 645487
Can someone help me confirm this is fixed? What do I need to test?
no confirmation needed
Status: RESOLVED → VERIFIED
Target Milestone: Firefox5 → Firefox 5
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: