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

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Bug 535408 comment 4 doesn't make sense; we couldn't remove it from firefox.js, as getIntPref would throw.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

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.

Comment 2

[Dão Gottwald [:dao]]

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.

Comment 3

[Dão Gottwald [:dao]]

Paul: ping?

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 421614 [details] [diff] [review]
patch

Sorry, slipped off my radar. r=zpao

Comment 5

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/6f48ddc83ea7

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Backout fixed Mac and Linux, no reason to believe it won't fix Windows too.

All the failed logs for reference:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264252531.1264254573.11593.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264255005.1264256901.6443.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264253887.1264256441.1209.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264241138.1264242702.2864.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264239387.1264241307.18454.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264239578.1264242260.30056.gz

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

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)]

Comment 9

[Dietrich Ayala (:dietrich)]

The changes look innocuous enough. The fact that this caused a crash is scary.

Comment 10

[Dão Gottwald [:dao]]

http://hg.mozilla.org/projects/cedar/rev/89ff348a5fe3

Comment 11

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/89ff348a5fe3

Comment 12

[u279076]

Can someone help me confirm this is fixed? What do I need to test?

Comment 13

[Dão Gottwald [:dao]]

no confirmation needed