Last Comment Bug 631447 - Intermittent infinite loop of "ASSERTION: XPConnect is being called on a scope without a 'Components' property!" in browser/components/places/tests/browser/browser_0_library_left_pane_migration.js winding up with "command timed out: 5400 seconds elapsed"
: Intermittent infinite loop of "ASSERTION: XPConnect is being called on a scop...
Status: RESOLVED FIXED
[test which aborts the suite]
: intermittent-failure, regression
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 6
Assigned To: Marco Bonardo [::mak]
:
Mentors:
: 630888 645102 646492 (view as bug list)
Depends on:
Blocks: 310955 438871 510489 619631 646492
  Show dependency treegraph
 
Reported: 2011-02-03 21:17 PST by Phil Ringnalda (:philor)
Modified: 2013-04-11 14:31 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1.0 (4.26 KB, patch)
2011-02-04 06:57 PST, Marco Bonardo [::mak]
gavin.sharp: review+
Details | Diff | Review
browser-base.content-mochitests-log (30.21 KB, text/plain)
2011-02-04 07:03 PST, Marco Bonardo [::mak]
no flags Details
patch for check-in (4.40 KB, patch)
2011-02-14 08:49 PST, Marco Bonardo [::mak]
no flags Details | Diff | Review
alternative approach (10.50 KB, patch)
2011-02-14 16:42 PST, Marco Bonardo [::mak]
no flags Details | Diff | Review
reduced patch v1.0 (5.33 KB, patch)
2011-03-29 08:12 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Review
reduced patch, slightly tweaked (8.13 KB, patch)
2011-04-25 14:49 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
mak77: feedback+
dao+bmo: feedback+
Details | Diff | Review
final patch (10.03 KB, patch)
2011-04-26 15:15 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Review

Description Phil Ringnalda (:philor) 2011-02-03 21:17:28 PST
This has been going on for around 10 days, but we've just been blowing it off, because digging around in 50MB logs for errors that aren't highlighted isn't much fun.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1296707279.1296713760.12672.gz
Rev3 Fedora 12x64 mozilla-central debug test mochitest-other on 2011/02/02 20:27:59
s: talos-r3-fed64-038

TEST-PASS | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Left pane version correctly set
++DOCSHELL 0x34b65a0 == 26
++DOMWINDOW == 128 (0x5aff008) [serial = 1810] [outer = (nil)]
WARNING: Subdocument container has no content: file /builds/slave/cen-lnx64-dbg/build/layout/base/nsDocumentViewer.cpp, line 2367
++DOMWINDOW == 129 (0x6853718) [serial = 1811] [outer = 0x5afef90]
WARNING: Subdocument container has no content: file /builds/slave/cen-lnx64-dbg/build/layout/base/nsDocumentViewer.cpp, line 2367
WARNING: NS_ENSURE_TRUE(sf) failed: file /builds/slave/cen-lnx64-dbg/build/docshell/base/nsDocShell.cpp, line 4985
WARNING: NS_ENSURE_TRUE(sf) failed: file /builds/slave/cen-lnx64-dbg/build/docshell/base/nsDocShell.cpp, line 4985
WARNING: Subdocument container has no content: file /builds/slave/cen-lnx64-dbg/build/layout/base/nsDocumentViewer.cpp, line 2367
###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!  (stack and details follow): 'Error', file /builds/slave/cen-lnx64-dbg/build/js/src/xpconnect/src/xpcwrappednativescope.cpp, line 761
DEBUG_CheckForComponentsInScope [js/src/xpconnect/src/xpcwrappednativescope.cpp:762]
XPCWrappedNativeScope::FindInJSObjectScope [js/src/xpconnect/src/xpcwrappednativescope.cpp:840]
XPCConvert::NativeInterface2JSObject [js/src/xpconnect/src/xpcconvert.cpp:1181]
XPCConvert::NativeData2JS [js/src/xpconnect/src/xpcconvert.cpp:492]
XPCConvert::NativeData2JS [js/src/xpconnect/src/xpcprivate.h:3211]
nsXPCWrappedJSClass::CallMethod [js/src/xpconnect/src/xpcwrappedjsclass.cpp:1594]
nsXPCWrappedJS::CallMethod [js/src/xpconnect/src/xpcwrappedjs.cpp:588]
PrepareAndDispatch [xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:153]
nsXPTCVariant::IsPtrData [xptcall.h:112]
The current JS stack is:
###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!  (stack and details follow): 'Error', file /builds/slave/cen-lnx64-dbg/build/js/src/xpconnect/src/xpcwrappednativescope.cpp, line 761
DEBUG_CheckForComponentsInScope [js/src/xpconnect/src/xpcwrappednativescope.cpp:762]
XPCWrappedNativeScope::FindInJSObjectScope [js/src/xpconnect/src/xpcwrappednativescope.cpp:840]
XPCConvert::NativeInterface2JSObject [js/src/xpconnect/src/xpcconvert.cpp:1181]
XPCConvert::NativeData2JS [js/src/xpconnect/src/xpcconvert.cpp:492]
XPCConvert::NativeData2JS [js/src/xpconnect/src/xpcprivate.h:3211]
nsXPCWrappedJSClass::CallMethod [js/src/xpconnect/src/xpcwrappedjsclass.cpp:1594]
nsXPCWrappedJS::CallMethod [js/src/xpconnect/src/xpcwrappedjs.cpp:588]
PrepareAndDispatch [xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:153]
nsXPTCVariant::IsPtrData [xptcall.h:112]
The current JS stack is:
###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!  (stack and details follow): 'Error', file /builds/slave/cen-lnx64-dbg/build/js/src/xpconnect/src/xpcwrappednativescope.cpp, line 761
DEBUG_CheckForComponentsInScope [js/src/xpconnect/src/xpcwrappednativescope.cpp:762]
XPCWrappedNativeScope::FindInJSObjectScope [js/src/xpconnect/src/xpcwrappednativescope.cpp:840]
XPCConvert::NativeInterface2JSObject [js/src/xpconnect/src/xpcconvert.cpp:1181]
XPCConvert::NativeData2JS [js/src/xpconnect/src/xpcconvert.cpp:492]
XPCConvert::NativeData2JS [js/src/xpconnect/src/xpcprivate.h:3211]
nsXPCWrappedJSClass::CallMethod [js/src/xpconnect/src/xpcwrappedjsclass.cpp:1594]
nsXPCWrappedJS::CallMethod [js/src/xpconnect/src/xpcwrappedjs.cpp:588]
PrepareAndDispatch [xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:153]
nsXPTCVariant::IsPtrData [xptcall.h:112]
The current JS stack is:
###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!  (stack and details follow): 'Error', file /builds/slave/cen-lnx64-dbg/build/js/src/xpconnect/src/xpcwrappednativescope.cpp, line 761
(repeats several times, until)
The current JS stack is:
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80570021: file /builds/slave/cen-lnx64-dbg/build/toolkit/components/places/src/nsNavBookmarks.cpp, line 2138
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80570021: file /builds/slave/cen-lnx64-dbg/build/toolkit/components/places/src/nsNavBookmarks.cpp, line 2897
(that line repeats dozens of times)
0 CB_create_query(aQueryUrl = "place:type=3&sort=4", aParentId = 92, aQueryName = "History") ["resource:///modules/PlacesUIUtils.jsm":1102]
    itemId = undefined
    this = [object Object]
    this.runBatched = [function]
    this.create_folder = [function]
    this.create_query = [function]
1 CB_runBatched(aUserData = null) ["resource:///modules/PlacesUIUtils.jsm":1153]
    this = [object Object]
    this.runBatched = [function]
    this.create_folder = [function]
    this.create_query = [function]
(etc., down to)
121 onload(event = [object Event @ 0x5f4d8f0 (native @ 0x474d870)]) ["chrome://browser/content/places/places.xul":1]
    this = [object ChromeWindow @ 0x61d1a40 (native @ 0x6853718)]
And the object whose scope lacks a 'Components' property is:
object 0x7f69f10b7958
class 0x7f6a1a132060 Object
flags: branded own_shape inDictionaryMode hasPropertyTable
properties:
    enumerate "_unstarredTooltip": slot 18
    enumerate "_ignoreClicks": slot 17
    enumerate "_itemIds": slot 16
    enumerate "_uri": slot 15
    enumerate "_starIcon": slot 14
    enumerate "onItemMoved": slot 13
    enumerate "onItemVisited": slot 12
    enumerate "onBeforeItemRemoved": slot 11
    enumerate "onEndUpdateBatch": slot 10
    enumerate "onBeginUpdateBatch": slot 9
    enumerate "onItemChanged": slot 8
    enumerate "onItemRemoved": slot 7
    enumerate "onItemAdded": slot 6
    enumerate "onClick": slot 5
    enumerate "_updateStateInternal": slot 4
    enumerate "updateState": slot 3
    enumerate getter shared "_starredTooltip": slot -1
    enumerate "QueryInterface": slot 2
    enumerate "uninit": slot 1
    enumerate "_hasBookmarksObserver": slot 0
proto <Object at 0x7f6a04c8fcf0>
parent <ChromeWindow object at 0x7f6a04c8fc60>
slots:
   0 = true
   1 = <function PSB_uninit (chrome://browser/content/browser.js:3383) at 0x7f69f1176160 (JSFunction at 0x7f69f2811500)>
   2 = <function XPCOMUtils_QueryInterface (resource://gre/modules/XPCOMUtils.jsm:254) at 0x7f69f1176268 (JSFunction at 0x7f6a05612200)>
   3 = <function PSB_updateState (chrome://browser/content/browser.js:3407) at 0x7f69f1176370 (JSFunction at 0x7f69f2811680)>
   4 = <function PSB__updateStateInternal (chrome://browser/content/browser.js:3445) at 0x7f69f11763c8 (JSFunction at 0x7f69f2811780)>
   5 = <function PSB_onClick (chrome://browser/content/browser.js:3461) at 0x7f69f1176420 (JSFunction at 0x7f69f2811800)>
   6 = <function PSB_onItemAdded (chrome://browser/content/browser.js:3472) at 0x7f69f1176478 (JSFunction at 0x7f69f2811880)>
   7 = <function PSB_onItemRemoved (chrome://browser/content/browser.js:3488) at 0x7f69f11764d0 (JSFunction at 0x7f69f2811900)>
   8 = <function PSB_onItemChanged (chrome://browser/content/browser.js:3503) at 0x7f69f1176528 (JSFunction at 0x7f69f2811980)>
   9 = <unnamed function (chrome://browser/content/browser.js:3526) at 0x7f69f1176580 (JSFunction at 0x7f69f2811a00)>
  10 = <unnamed function (chrome://browser/content/browser.js:3527) at 0x7f69f11765d8 (JSFunction at 0x7f69f2811a80)>
  11 = <unnamed function (chrome://browser/content/browser.js:3528) at 0x7f69f1176630 (JSFunction at 0x7f69f2811b00)>
  12 = <unnamed function (chrome://browser/content/browser.js:3529) at 0x7f69f1176688 (JSFunction at 0x7f69f2811b80)>
  13 = <unnamed function (chrome://browser/content/browser.js:3530) at 0x7f69f11766e0 (JSFunction at 0x7f69f2811c00)>
  14 = <XULElement object at 0x7f69f1d67ec8>
  15 = <XPCWrappedNative_NoHelper object at 0x7f69cfe4b478>
  16 = <Array object at 0x7f69f1121aa0>
  17 = false
  18 = "Bookmark this page"

at which point the loop starts over again, down to

Output exceeded 52428800 bytes, remaining output has been truncated

command timed out: 5400 seconds elapsed, killing pid 2128

I'm torn between places and xpconnect, so filing in Core: Dumping Ground and cc'ing both.
Comment 1 Andreas Gal :gal 2011-02-03 21:24:28 PST
Do we have a regression range?
Comment 2 Phil Ringnalda (:philor) 2011-02-03 21:35:28 PST
Note that because it's pretty much impossible to load the full log in the browser, and the brief log doesn't actually show much other than a random bit of the loop just before the timeout, it's possible that anything which I call this will actually not be, or will be this but started by a different test, or possibly will be meatcake.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1296768758.1296775425.23948.gz#err2
Rev3 Fedora 12 mozilla-central debug test mochitest-other on 2011/02/03 13:32:38
s: talos-r3-fed-031
Comment 3 Phil Ringnalda (:philor) 2011-02-03 21:37:48 PST
I'll be working my way slowly back to the start, but the reason it's gotten away with not being filed for two weeks is because it only hits a couple times a day, so the range is going to be pretty roughly "that weekend I took off back in January, I think the 22nd/23rd."
Comment 4 Phil Ringnalda (:philor) 2011-02-03 22:49:30 PST
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1296670476.1296677413.31396.gz
Rev3 WINNT 6.1 mozilla-central debug test mochitest-other on 2011/02/02 10:14:36
s: talos-r3-w7-016

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1296513481.1296520057.15075.gz
Rev3 Fedora 12 mozilla-central debug test mochitest-other on 2011/01/31 14:38:01
s: talos-r3-fed-051

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1296496508.1296503055.21660.gz
Rev3 MacOSX Snow Leopard 10.6.2 mozilla-central debug test mochitest-other on 2011/01/31 09:55:08
s: talos-r3-snow-020

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1296340606.1296347500.10493.gz
Rev3 WINNT 6.1 mozilla-central debug test mochitest-other on 2011/01/29 14:36:46
s: talos-r3-w7-022

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1296209517.1296216319.29168.gz
Rev3 MacOSX Snow Leopard 10.6.2 mozilla-central debug test mochitest-other on 2011/01/28 02:11:57
s: talos-r3-snow-050

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1296176733.1296183918.9117.gz
Rev3 WINNT 6.1 mozilla-central debug test mochitest-other on 2011/01/27 17:05:33
s: talos-r3-w7-031

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1296170503.1296179076.17439.gz
Rev3 MacOSX Snow Leopard 10.6.2 mozilla-central debug test mochitest-other on 2011/01/27 15:21:43
s: talos-r3-snow-053

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1295997584.1296004090.314.gz
Rev3 Fedora 12 mozilla-central debug test mochitest-other on 2011/01/25 15:19:44
s: talos-r3-fed-049

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1295846361.1295852925.5978.gz
Rev3 Fedora 12 mozilla-central debug test mochitest-other on 2011/01/23 21:19:21
s: talos-r3-fed-016

I didn't expect much of the regression window, since it was going to have to be a fuzzy 2-3 days before the first one, but, that first instance was on http://tbpl.mozilla.org/?rev=bc2fda9a8b32 which includes the obvious cause of an infinite loop while dumping the JS stack, since it includes the cset that dumps the JS stack.
Comment 5 Marco Bonardo [::mak] 2011-02-04 05:01:30 PST
hm, interesting failures.
This is the batch that creates the left pane of the Library, it is failing due to a call to GetBookmarkURI that I have not yet identified, that fails on a NS_newURI.
The object without a scope is PlacesStarButton, that starts observing bookmarks onLocationChange and stops at BrowserShutdown.
I have to note this is the first Places b-c test that runs in our harness, so it's even possible a bad interaction with the tests that runs just before it, I see base/content/test/tabview/browser_tabview_undo_group.js and components/certerror/test/browser_bug431826.js
If I run the test alone I see some warning but not this kind of scope missing loop.
Comment 6 Marco Bonardo [::mak] 2011-02-04 05:11:36 PST
After "Left pane version correctly set" message, we register a listener to the window watcher and open the Library window, at this point we wait for "load" event, onload the library initializes the left pane that runs a batch that creates some bookmarks. We are at the point where the assertion happens. The star button object is most likely listening, but should be in the browser scope, unless this is a ghost object left around by a missing call to BrowserShutdown, but that is called in the onunload handler of the browser window.
It's possible a previous test causes some part of BrowserShutdown to throw before running to completion. A bunch of calls there should be wrapped by a try/catch.
Comment 7 Marco Bonardo [::mak] 2011-02-04 05:14:26 PST
previous tests are showing a bunch of "reference to undefined property aBrowser.webNavigation"
Comment 8 Marco Bonardo [::mak] 2011-02-04 05:17:17 PST
ah-ha here is the problem:

TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_undo_group.js | Console message: [JavaScript Error: "uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://browser/content/browser.js :: BrowserShutdown :: line 6885"  data: no]"]

a Panorama b-c test (looks like browser_tabview_undo_group.js) is causing an uncaught assertion in BrowserShutdown

looks like one of these is failing
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1691
1691   Services.obs.removeObserver(gSessionHistoryObserver, "browser:purge-session-history");
1692   Services.obs.removeObserver(gXPInstallObserver, "addon-install-disabled");
1693   Services.obs.removeObserver(gXPInstallObserver, "addon-install-started");
1694   Services.obs.removeObserver(gXPInstallObserver, "addon-install-blocked");
1695   Services.obs.removeObserver(gXPInstallObserver, "addon-install-failed");
1696   Services.obs.removeObserver(gXPInstallObserver, "addon-install-complete");
1697   Services.obs.removeObserver(gPluginHandler.pluginCrashed, "plugin-crashed");
1698   Services.obs.removeObserver(gFormSubmitObserver, "invalidformsubmit");
Comment 9 Marco Bonardo [::mak] 2011-02-04 05:27:25 PST
It's even possible I blamed the wrong test and that browser_tabview_undo_group.js is only bringing the problem to evidence but it's a previous test causing it (by removing a global browser observer).

I'll add some try catch and try to see if I can find the culprit topic. Taking for now.
Comment 10 Marco Bonardo [::mak] 2011-02-04 06:57:42 PST
Created attachment 509756 [details] [diff] [review]
patch v1.0

I think we could want to do something like this in the shutdown function, to ensure in future people won't add stuff that could interrupt the cleanup.
This way instead of getting a looping error we would get a notification, still usable to investigate issues through the log.

The way and the name of the helper are not finalized, feedback welcome.

I'll follow with the log after a run of browser/base/content tests (I think the problem lives here).
Comment 11 Marco Bonardo [::mak] 2011-02-04 07:03:02 PST
Created attachment 509758 [details]
browser-base.content-mochitests-log

There are three tests printing out shutdown errors in my last run in browser/base/content:

browser_bug491431.js
browser_tab_dragdrop2.js
browser_tabview_undo_group.js
Comment 12 Marco Bonardo [::mak] 2011-02-04 07:33:02 PST
also

browser_bug609700.js
Comment 13 Treeherder Robot 2011-02-06 14:49:29 PST
ehsan%mozilla.com
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297024216.1297031065.12794.gz
Rev3 WINNT 6.1 mozilla-central debug test mochitest-other on 2011/02/06 12:30:16

s: talos-r3-w7-016
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | We correctly have only 1 left pane folder - Got 0, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | leftPaneFolderId getter has correct value - Got undefined, expected 95
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_410196_paste_into_tags.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_423515.js | has new tag - Got 2, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_bookmarksProperties.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_library_batch_delete.js | Test timed out
buildbot.slave.commands.TimeoutError: command timed out: 5400 seconds elapsed, killing pid 2628
Comment 15 Treeherder Robot 2011-02-08 20:39:50 PST
philringnalda%gmail.com
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297217780.1297224335.26200.gz
Rev3 MacOSX Leopard 10.5.8 mozilla-central debug test mochitest-other on 2011/02/08 18:16:20

s: talos-r3-leopard-004
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | leftPaneFolderId getter has correct value - Got 324, expected 93
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Test timed out
buildbot.slave.commands.TimeoutError: command timed out: 5400 seconds elapsed, killing pid 316
Comment 16 Treeherder Robot 2011-02-09 04:48:25 PST
mak77%bonardo.net
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297247362.1297253884.31536.gz
Rev3 Fedora 12 mozilla-central debug test mochitest-other on 2011/02/09 02:29:22

s: talos-r3-fed-007
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | We correctly have only 1 left pane folder - Got 0, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | leftPaneFolderId getter has correct value - Got undefined, expected 93
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Test timed out
buildbot.slave.commands.TimeoutError: command timed out: 5400 seconds elapsed, killing pid 2049
Comment 17 Phil Ringnalda (:philor) 2011-02-09 13:45:35 PST
http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1297279047.1297285563.8794.gz
Rev3 Fedora 12 tracemonkey debug test mochitest-other on 2011/02/09 11:17:27
s: talos-r3-fed-022
Comment 18 Phil Ringnalda (:philor) 2011-02-11 17:52:21 PST
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297468461.1297475029.22474.gz
Rev3 MacOSX Snow Leopard 10.6.2 mozilla-central debug test mochitest-other on 2011/02/11 15:54:21
s: talos-r3-snow-053
Comment 19 Phil Ringnalda (:philor) 2011-02-13 19:09:32 PST
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297645826.1297652637.13069.gz
Rev3 WINNT 6.1 mozilla-central debug test mochitest-other on 2011/02/13 17:10:26
s: talos-r3-w7-027
Comment 20 Marco Bonardo [::mak] 2011-02-14 08:49:36 PST
Created attachment 512170 [details] [diff] [review]
patch for check-in

Ok, here is the patch ready for check-in.

As a plan of action, once this bug is fixed, I'll open a meta bug on BrowserShutdown exceptions, and attach a dependent bug on each test causing them.
Comment 21 Phil Ringnalda (:philor) 2011-02-14 14:38:39 PST
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297714913.1297721492.9935.gz
Rev3 MacOSX Leopard 10.5.8 mozilla-central debug test mochitest-other on 2011/02/14 12:21:53
s: talos-r3-leopard-015
Comment 22 Phil Ringnalda (:philor) 2011-02-14 14:41:01 PST
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297716275.1297722811.17575.gz
Rev3 MacOSX Snow Leopard 10.6.2 mozilla-central debug test mochitest-other on 2011/02/14 12:44:35
s: talos-r3-snow-029
Comment 23 Marco Bonardo [::mak] 2011-02-14 16:42:09 PST
Created attachment 512342 [details] [diff] [review]
alternative approach

This is an alternative approach that I briefly proposed to Gavin on IRC.
The problem with the above patch is that I still see a bunch of assertions in those tests where we call BrowserShutdown before DelayedStartup.

Managing all of this at shutdown is a lot error-prone both for current and future code, and makes things less readable. I suggested to proceed like we do in BrowserTest harness, provide a RegisterBrowserShutdownFunction() global and use it everytime something that needs cleanup is initialized.
This has 2 advantages:
- finalizers are near initializers, thus the blame for finalizing stuff is clear.
- anything that has not been initialized won't be finalized

With this patch I ran browser_privatebrowsing_windowTitle.js test, this was usually throwing a lot of assertions, after patching there is none.

I'll run this through tryserver.
Comment 24 Marco Bonardo [::mak] 2011-02-15 03:19:40 PST
Comment on attachment 512342 [details] [diff] [review]
alternative approach

passed try
Comment 25 Treeherder Robot 2011-02-15 11:01:12 PST
philringnalda%gmail.com
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297789156.1297795693.10772.gz
Rev3 MacOSX Leopard 10.5.8 mozilla-central debug test mochitest-other on 2011/02/15 08:59:16

s: talos-r3-leopard-024
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | leftPaneFolderId getter has correct value - Got 325, expected 94
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Test timed out
buildbot.slave.commands.TimeoutError: command timed out: 5400 seconds elapsed, killing pid 359
Comment 26 Phil Ringnalda (:philor) 2011-02-15 14:44:46 PST
http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1297801781.1297808707.1084.gz
Rev3 WINNT 6.1 tracemonkey debug test mochitest-other on 2011/02/15 12:29:41
s: talos-r3-w7-013
Comment 27 Phil Ringnalda (:philor) 2011-02-16 14:20:53 PST
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297878377.1297884961.19101.gz
Rev3 MacOSX Leopard 10.5.8 mozilla-central debug test mochitest-other on 2011/02/16 09:46:17
s: talos-r3-leopard-039
Comment 28 Treeherder Robot 2011-02-17 07:35:15 PST
philringnalda%gmail.com
http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1297945154.1297951688.30657.gz
Rev3 MacOSX Snow Leopard 10.6.2 tracemonkey debug test mochitest-other on 2011/02/17 04:19:14

s: talos-r3-snow-028
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | We correctly have only 1 left pane folder - Got 0, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | leftPaneFolderId getter has correct value - Got undefined, expected 94
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Test timed out
buildbot.slave.commands.TimeoutError: command timed out: 5400 seconds elapsed, killing pid 507
Comment 29 Phil Ringnalda (:philor) 2011-02-17 10:49:33 PST
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297960457.1297967000.32591.gz
Rev3 MacOSX Leopard 10.5.8 mozilla-central debug test mochitest-other on 2011/02/17 08:34:17
s: talos-r3-leopard-013
Comment 30 Treeherder Robot 2011-02-17 11:07:40 PST
philringnalda%gmail.com
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297962897.1297969408.10412.gz
Rev3 MacOSX Snow Leopard 10.6.2 mozilla-central debug test mochitest-other on 2011/02/17 09:14:57

s: talos-r3-snow-014
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Test timed out
buildbot.slave.commands.TimeoutError: command timed out: 5400 seconds elapsed, killing pid 340
Comment 31 Treeherder Robot 2011-02-17 22:06:34 PST
philringnalda%gmail.com
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1298001626.1298008427.17264.gz
Rev3 WINNT 6.1 mozilla-central debug test mochitest-other on 2011/02/17 20:00:26

s: talos-r3-w7-029
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | leftPaneFolderId getter has correct value - Got 314, expected 94
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_410196_paste_into_tags.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_423515.js | has new tag - Got 2, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_bookmarksProperties.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_library_batch_delete.js | Found folder in content pane - Got bookmark/http://example.com/, expected keepme
12 = <unnamed function (chrome://browser/content/browser.js:3541) at 084A1D5TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_library_batch_delete.js | Right pane was correctly updated - Got 3, expected 2
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_library_batch_delete.js | Test timed out
buildbot.slave.commands.TimeoutError: command timed out: 5400 seconds elapsed, killing pid 3732
Comment 32 Phil Ringnalda (:philor) 2011-02-21 02:29:48 PST
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1298276956.1298283517.6122.gz
Rev3 Fedora 12 mozilla-central debug test mochitest-other on 2011/02/21 00:29:16
s: talos-r3-fed-037
Comment 33 Marco Bonardo [::mak] 2011-02-22 16:00:27 PST
*** Bug 630888 has been marked as a duplicate of this bug. ***
Comment 34 Phil Ringnalda (:philor) 2011-02-23 09:20:17 PST
http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1298457556.1298464360.4270.gz
Rev3 WINNT 6.1 tracemonkey debug test mochitest-other on 2011/02/23 02:39:16
s: talos-r3-w7-035
Comment 35 Mike Beltzner [:beltzner, not reading bugmail] 2011-02-23 18:31:56 PST
Comment on attachment 509756 [details] [diff] [review]
patch v1.0

(bookkeeping)
Comment 36 Phil Ringnalda (:philor) 2011-02-24 08:08:40 PST
http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1298556526.1298563038.12029.gz
Rev3 MacOSX Leopard 10.5.8 tracemonkey debug test mochitest-other on 2011/02/24 06:08:46
s: talos-r3-leopard-041
Comment 37 Phil Ringnalda (:philor) 2011-02-25 07:19:40 PST
http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1298613097.1298619626.23377.gz
Rev3 Fedora 12x64 tracemonkey debug test mochitest-other on 2011/02/24 21:51:37
s: talos-r3-fed64-013
Comment 38 Dão Gottwald [:dao] 2011-02-28 06:11:14 PST
Please don't let the term shutdown slip into new API. The browser isn't shutting down here, but a browser window is closing.
Comment 39 Marco Bonardo [::mak] 2011-02-28 06:23:59 PST
(In reply to comment #38)
> Please don't let the term shutdown slip into new API. The browser isn't
> shutting down here, but a browser window is closing.

makes sense, what about RegisterOnWindowCloseHandler? At this point we could delay this fix to post-fx4 and also rename BrowserShutdown.
Comment 40 Dão Gottwald [:dao] 2011-02-28 06:30:06 PST
(In reply to comment #39)
> makes sense, what about RegisterOnWindowCloseHandler?

Seems a bit clunky. How about addEventListener("unload", ...)? :)
Comment 41 Treeherder Robot 2011-02-28 07:26:44 PST
philringnalda%gmail.com
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1298896667.1298903623.30687.gz
Rev3 WINNT 6.1 mozilla-central debug test mochitest-other on 2011/02/28 04:37:47

s: talos-r3-w7-019
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | leftPaneFolderId getter has correct value - Got 315, expected 94
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Test timed out
buildbot.slave.commands.TimeoutError: command timed out: 5400 seconds elapsed, killing pid 680
Comment 42 Marco Bonardo [::mak] 2011-02-28 09:13:35 PST
(In reply to comment #40)
> (In reply to comment #39)
> > makes sense, what about RegisterOnWindowCloseHandler?
> 
> Seems a bit clunky. How about addEventListener("unload", ...)? :)

A lot of these registrations could be in the startup path, using addEventListener is more expensive than just adding a function to an array and have a single event listener. Also here I'd like to wrap each finalization in a try catch to make "shutdown" more reliable and have meaningful errors, so using a registration function seems simpler.
Comment 43 Dão Gottwald [:dao] 2011-02-28 10:01:50 PST
(In reply to comment #42)
> A lot of these registrations could be in the startup path, using
> addEventListener is more expensive than just adding a function to an array and
> have a single event listener.

The changes in BrowserStartup aren't needed anyway, as the load event is guaranteed to occur before the unload event.

> Also here I'd like to wrap each finalization in a
> try catch to make "shutdown" more reliable and have meaningful errors, so using
> a registration function seems simpler.

I don't think I see the need. A call to an uninit function shouldn't fail, as long as the corresponding init function was called beforehand.
Comment 44 Phil Ringnalda (:philor) 2011-02-28 16:08:15 PST
http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1298930953.1298937562.21522.gz
Rev3 MacOSX Leopard 10.5.8 tracemonkey debug test mochitest-other on 2011/02/28 14:09:13
s: talos-r3-leopard-016
Comment 47 Treeherder Robot 2011-03-08 11:48:39 PST
philringnalda%gmail.com
http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1299607490.1299609780.14716.gz
Rev3 MacOSX Snow Leopard 10.6.2 tracemonkey debug test mochitest-other on 2011/03/08 10:04:50

s: talos-r3-snow-011
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Exited with code 1 during test run
PROCESS-CRASH | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | application crashed (minidump found)
Thread 0 (crashed)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | missing output line for total leaks!
TEST-UNEXPECTED-FAIL | plugin process 376 | automationutils.processLeakLog() | missing output line for total leaks!
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
Comment 48 Treeherder Robot 2011-03-09 16:39:13 PST
philringnalda%gmail.com
http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1299717186.1299715550.21484.gz
Rev3 MacOSX Leopard 10.5.8 tracemonkey debug test mochitest-other on 2011/03/09 16:33:06

s: talos-r3-leopard-005
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Exited with code 1 during test run
PROCESS-CRASH | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | application crashed (minidump found)
Thread 0 (crashed)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | missing output line for total leaks!
Comment 49 Marco Bonardo [::mak] 2011-03-10 07:52:58 PST
(In reply to comment #43)
> (In reply to comment #42)
> The changes in BrowserStartup aren't needed anyway, as the load event is
> guaranteed to occur before the unload event.

> I don't think I see the need. A call to an uninit function shouldn't fail, as
> long as the corresponding init function was called beforehand.

Ideally this bug is not needed as well, but it happened because the way this is done now is confusing and hard to report.
Breaking init/uninit stuff is easy, but noticing and reporting the breakage is not, that is what this tries to handle.
Thus I'm trying to find alternatives to better associate window-close stuff with relative window-open stuff, and to better report errors (So that bugs can be filed earlier and with more context).
Now, we can figure out a better approach, even better than mine, but the problem exists.
Comment 50 Dão Gottwald [:dao] 2011-03-10 07:59:21 PST
The current setup isn't really confusing or even wrong as far as I can see. It just assumes that windows aren't closed before delayedStartup is called, which is a sane assumption for Firefox in the wild but not for tests.
Comment 51 Dão Gottwald [:dao] 2011-03-10 08:02:40 PST
Also note that addEventListener("load", ...) accompanied by addEventListener("unload", ...) is the common pattern used by extensions.
Comment 52 Marco Bonardo [::mak] 2011-03-10 08:08:11 PST
I don't think current setup is plain wrong, just it doesn't help bug reporters.
I guess I'll make a minimal patch (just handle clearTimeout and move shutdown for delayed stuff in a condition) to fix this issue early in the 4.next reopening, and then we can discuss the rest apart.
Comment 53 Treeherder Robot 2011-03-10 17:52:00 PST
philringnalda%gmail.com
http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1299803201.1299804847.10000.gz
Rev3 MacOSX Leopard 10.5.8 tracemonkey debug test mochitest-other on 2011/03/10 16:26:41

s: talos-r3-leopard-026
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Exited with code 1 during test run
PROCESS-CRASH | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | application crashed (minidump found)
Thread 0 (crashed)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | missing output line for total leaks!
Comment 54 Phil Ringnalda (:philor) 2011-03-10 21:38:09 PST
Could we please have whichever whatever patch, in a sort of a desperate hurry?

About half of the newly added WinXP debug runs (http://tbpl.mozilla.org/?noignore=1 since they're hidden for a permaorange in mochitest-chrome) are hitting this, the half that aren't hitting bug 629610 and getting taken out before this can hit, and according to bug 614955 comment 42 instead of just creating a 50MB log and then tying up the slave for 90 minutes before timing out, they may be tying up the slave for 20 hours.
Comment 55 Marco Bonardo [::mak] 2011-03-11 05:29:28 PST
(In reply to comment #54)
> Could we please have whichever whatever patch, in a sort of a desperate hurry?

I think even if I do a patch in the next 2 seconds, it won't be allowed to land, we cannot land a browser change till the tree reopens for check-ins that are not fennec-only. Btw, plan to to do it today.
Comment 56 :Ehsan Akhgari (busy, don't ask for review please) 2011-03-11 17:18:50 PST
(In reply to comment #55)
> (In reply to comment #54)
> > Could we please have whichever whatever patch, in a sort of a desperate hurry?
> 
> I think even if I do a patch in the next 2 seconds, it won't be allowed to
> land, we cannot land a browser change till the tree reopens for check-ins that
> are not fennec-only. Btw, plan to to do it today.

FWIW, people have been breaking this rule by landing all sorts of (most likely) unnecessary test fixes ever since we've closed the tree down to only blockers, so if you can come up with a patch which fixes this, you have my approval on landing it (and yes, you can quote me on this!)  :D
Comment 57 Dão Gottwald [:dao] 2011-03-11 17:39:55 PST
NPOTB test fixes can land, of course. This isn't going to be such a fix.
Comment 58 :Ehsan Akhgari (busy, don't ask for review please) 2011-03-11 18:49:20 PST
(In reply to comment #57)
> NPOTB test fixes can land, of course. This isn't going to be such a fix.

Well, I'll shut my mouth then.  :-)
Comment 60 Treeherder Robot 2011-03-16 19:32:23 PDT
philringnalda%gmail.com
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1300317557.1300324273.25468.gz
Rev3 MacOSX Snow Leopard 10.6.2 mozilla-central debug test mochitest-other on 2011/03/16 16:19:17

s: talos-r3-snow-011
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_overflowScroll.js | Scrolled one tab to the left with a single click - Got 155, expected 55
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | leftPaneFolderId getter has correct value - Got 417, expected 93
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Test timed out
buildbot.slave.commands.TimeoutError: command timed out: 5400 seconds elapsed, killing pid 388
Comment 63 Treeherder Robot 2011-03-27 14:16:24 PDT
ehsan%mozilla.com
http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1301249709.1301256231.9781.gz
Rev3 MacOSX Snow Leopard 10.6.2 cedar debug test mochitest-other on 2011/03/27 11:15:09

s: talos-r3-snow-019
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | leftPaneFolderId getter has correct value - Got 340, expected 17
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Test timed out
buildbot.slave.commands.TimeoutError: command timed out: 5400 seconds elapsed, killing pid 533
Comment 64 Marco Bonardo [::mak] 2011-03-28 08:42:26 PDT
*** Bug 645102 has been marked as a duplicate of this bug. ***
Comment 65 Treeherder Robot 2011-03-28 15:34:15 PDT
ehsan%mozilla.com
http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1301340917.1301347759.10522.gz
Rev3 MacOSX Snow Leopard 10.6.2 cedar debug test mochitest-other on 2011/03/28 12:35:17

s: talos-r3-snow-039
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | leftPaneFolderId getter has correct value - Got 341, expected 17
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Test timed out
buildbot.slave.commands.TimeoutError: command timed out: 5400 seconds elapsed, killing pid 335
Comment 66 Marco Bonardo [::mak] 2011-03-29 08:12:13 PDT
Created attachment 522678 [details] [diff] [review]
reduced patch v1.0

This is the basic idea, to avoid doing too many changes here (I avoided to do any change to the uninit calls themselves).
It just tracks if delayed startup did run, and avoids running delayed shutdown tasks if it didn't.
Before going for review I still have to run tests, but attaching nontheless to collect eventual earlier feedback.
Comment 67 Marco Bonardo [::mak] 2011-03-30 16:44:53 PDT
Comment on attachment 522678 [details] [diff] [review]
reduced patch v1.0

local testing was fine.
Comment 68 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-03-31 14:34:55 PDT
Use an early return to avoid the indentation changes? Would make this a lot easier to review.
Comment 69 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-03-31 14:36:07 PDT
Oh, I guess that'd require re-ordering stuff anyways, nevermind.
Comment 70 Marco Bonardo [::mak] 2011-03-31 15:36:39 PDT
yes, I should still reorder stuff, if you want an easier patch I could make a patch with ignoring whitespaces, it would save some line. Btw, I mostly copy/pasted preserving original calls ordering to avoid surprises.
Comment 72 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-02 10:33:48 PDT
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1301762253.1301764234.17042.gz&fulltext=1
Comment 74 Treeherder Robot 2011-04-08 13:39:49 PDT
peterv
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1302287006.1302293728.16974.gz
Rev3 MacOSX Snow Leopard 10.6.2 mozilla-central debug test mochitest-other on 2011/04/08 11:23:26

s: talos-r3-snow-040
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | We correctly have only 1 left pane folder - Got 0, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | leftPaneFolderId getter has correct value - Got undefined, expected 17
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Found a Places:Organizer after previous test timed out
buildbot.slave.commands.TimeoutError: command timed out: 5400 seconds elapsed, attempting to kill
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
Comment 75 Treeherder Robot 2011-04-08 13:40:01 PDT
dherman
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1302287006.1302293728.16974.gz
Rev3 MacOSX Snow Leopard 10.6.2 mozilla-central debug test mochitest-other on 2011/04/08 11:23:26

s: talos-r3-snow-040
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | We correctly have only 1 left pane folder - Got 0, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | leftPaneFolderId getter has correct value - Got undefined, expected 17
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Found a Places:Organizer after previous test timed out
buildbot.slave.commands.TimeoutError: command timed out: 5400 seconds elapsed, attempting to kill
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
Comment 76 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-09 08:39:58 PDT
http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1302339447.1302346578.14390.gz
Comment 77 Treeherder Robot 2011-04-12 09:14:51 PDT
philor
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1302617111.1302623775.2363.gz
Rev3 MacOSX Snow Leopard 10.6.2 mozilla-central debug test mochitest-other on 2011/04/12 07:05:11

s: talos-r3-snow-048
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | We correctly have only 1 left pane folder - Got 0, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | leftPaneFolderId getter has correct value - Got undefined, expected 17
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Found a Places:Organizer after previous test timed out
buildbot.slave.commands.TimeoutError: command timed out: 5400 seconds elapsed, attempting to kill
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
Comment 80 Treeherder Robot 2011-04-18 20:20:12 PDT
philor
http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1303175844.1303182454.20632.gz
Rev3 MacOSX Snow Leopard 10.6.2 cedar debug test mochitest-other on 2011/04/18 18:17:24

s: talos-r3-snow-049
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | We correctly have only 1 left pane folder - Got 0, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | leftPaneFolderId getter has correct value - Got undefined, expected 17
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Found a Places:Organizer after previous test timed out
buildbot.slave.commands.TimeoutError: command timed out: 5400 seconds elapsed, attempting to kill
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
PROCESS-CRASH | Main app process exited normally | application crashed (minidump found)
Thread 0 (crashed)
Comment 83 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-25 12:02:00 PDT
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1303749311.1303756280.9960.gz
Comment 84 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-04-25 14:49:01 PDT
Created attachment 528183 [details] [diff] [review]
reduced patch, slightly tweaked

I found that the easiest way to review this was to write it myself and then compare to yours - turns out our patches are almost identical, which is a good sign! (I snuck in some minor addition style/comment tweaks). I also audited the uninit methods being called to ensure there were no hidden dependencies.

In testing this, I discovered another case we need to handle: unload can be called before load does, if you call close() before the load is complete, so we need to handle that too. In this case, I just made BrowserShutdown a no-op.

My tests were basically using:
var win=OpenBrowserWindow(); setTimeout(function () {win.close();}, SOMETIMEOUT);

For me, SOMETIMEOUT of 100ms was enough to hit the second case (unload firing before load), and 150ms was enough to catch the other condition (after load, but before delayedStartup).
Comment 87 Marco Bonardo [::mak] 2011-04-26 04:06:41 PDT
Comment on attachment 528183 [details] [diff] [review]
reduced patch, slightly tweaked

It's fine, minor changes mostly.

In the nonBrowserWindowShutdown case there is only delayed shutdown to do, so you didn't use gStartupRan, personally I'd still set it to true in nonBrowserWindowStartup though, so that if somebody in future uses it it's correctly setup.

In browser.js the bracing coding style is mixed up, since I see you moved braces to same line as function definitions I assume this is the wanted style in browser.js, you should then probably also fix nonBrowserWindowStartup style.
Comment 88 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-04-26 15:15:38 PDT
Created attachment 528459 [details] [diff] [review]
final patch

Final patch, ready for checkin. Passed tests on try.
Comment 89 Dão Gottwald [:dao] 2011-04-27 01:01:00 PDT
http://hg.mozilla.org/mozilla-central/rev/8deabe91c24b
Comment 95 Robert Strong [:rstrong] (use needinfo to contact me) 2011-05-25 15:50:25 PDT
http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Beta/1306354218.1306361628.13690.gz
Comment 96 Matt Brubeck (:mbrubeck) 2011-06-07 13:41:23 PDT
*** Bug 646492 has been marked as a duplicate of this bug. ***
Comment 98 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-04-11 14:30:45 PDT
*** Bug 310955 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.