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)
Firefox
Session Restore
Tracking
()
VERIFIED
FIXED
Firefox 5
People
(Reporter: dao, Assigned: dao)
References
Details
Attachments
(1 file)
4.32 KB,
patch
|
zpao
:
review+
|
Details | Diff | Splinter 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 1•14 years ago
|
||
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.
Assignee | ||
Comment 2•14 years ago
|
||
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.
Assignee | ||
Comment 3•14 years ago
|
||
Paul: ping?
Comment 4•14 years ago
|
||
Comment on attachment 421614 [details] [diff] [review] patch Sorry, slipped off my radar. r=zpao
Attachment #421614 -
Flags: review?(paul) → review+
Assignee | ||
Comment 5•14 years ago
|
||
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 → ---
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•14 years ago
|
||
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•14 years ago
|
||
The changes look innocuous enough. The fact that this caused a crash is scary.
Assignee | ||
Comment 10•13 years ago
|
||
http://hg.mozilla.org/projects/cedar/rev/89ff348a5fe3
Whiteboard: fixed-in-cedar
Target Milestone: Firefox 3.7a1 → ---
Comment 11•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/89ff348a5fe3
Status: REOPENED → RESOLVED
Closed: 14 years ago → 13 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Assignee | ||
Updated•13 years ago
|
Target Milestone: --- → Firefox4.2
Comment 12•13 years ago
|
||
Can someone help me confirm this is fixed? What do I need to test?
Assignee | ||
Comment 13•13 years ago
|
||
no confirmation needed
Assignee | ||
Updated•13 years ago
|
Target Milestone: Firefox5 → Firefox 5
You need to log in
before you can comment on or make changes to this bug.
Description
•