Closed Bug 483980 Opened 16 years ago Closed 15 years ago

Allow history/bookmark observer components to register with a startup category

Categories

(Toolkit :: Places, defect)

Product:
Toolkit
Component:
Places
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.2a1
Tracking Flags:
Tracking Status
blocking1.9.1 --- -
status1.9.1 --- .3-fixed
fennec 1.0b3+ ---

People

(Reporter: mfinkle, Assigned: sdwilsh)

References

Details

(Keywords: mobile, perf, verified1.9.1)

Attachments

(2 files, 20 obsolete files)

patch v6.7
(deleted), patch
Details | Diff | Splinter Review
1.9.1 branch patch
(deleted), patch
samuel.sidler+old
: approval1.9.1.3+
Details | Diff | Splinter Review
Attached patch patch 1 (obsolete) (deleted) — — Splinter Review 
History/Bookmark observers like nsPlacesDBFlush use a startup category like "profile-after-change" to get XPCOM to load them, at which point they register themselves with the history or bookmark service. The component does this so it is registered as an observer as soon as possible - they don't want to miss any notifications.

Doing this causes the history and bookmark services to be created and initialized very early during startup, affecting performance.

This patch creates "history-observers" and "bookmark-observers" startup categories. The history and bookmark services load components in these categories before firing any notifications. We can then delay the creation of the observers until the latest possible moment.

(sorry for the whitespace changes)
Keywords: mobile, perf
tracking-fennec: --- → ?
I tested the patch on Windows desktop first. It does delay the loading of nsPlacesDBFlush until after a bookmark observer notification is fired. After that, the DB flush timer is operational.

I am building on scratchbox and testing on an n810 next.
Attached patch patch 1 - with no whitespace changes (obsolete) (deleted) — — Splinter Review 
Same as previous patch, but without whitespace changes
Assignee: nobody → mark.finkle
Attachment #368003 - Attachment is obsolete: true
Comment on attachment 368015 [details] [diff] [review]
patch 1 - with no whitespace changes

Shawn - Can you take a quick look to make sure this is what you were thinking?
Attachment #368015 - Flags: review?(sdwilsh)
Comment on attachment 368015 [details] [diff] [review]
patch 1 - with no whitespace changes

I was thinking that they wouldn't have to then register themselves with the service bookmark or history service.  I think you'll need to make this change 

A few issues with your solution though:
1) Can we make a new inline function that does the call to the category manager and enumerates the listener arrays please?  Something like NS_PlacesNotifyListeners.
2) nsPlacesDBFlush needs to have the category for history too, so if we add stuff to history, we can start our timer.  That also means we do not want to get the bookmarks service right off the bat too.
3) Can you add a test that ensures that components registered in a category are in fact created and added as observers for history and bookmarks?
Attachment #368015 - Flags: review?(sdwilsh) → review-
Attached patch patch 2 (obsolete) (deleted) — — Splinter Review 
This patch:
* uses a macro to enumerate the category observers
* category observers no longer need to register with the service
* adds a macro to a new file, nsPlacesMacros.h
* modifies nsPlacesDBFlush.js:
  * no longer adds itself to the bookmark service
  * adds "history-observer" category
  * adds support for the history observer interface
Attachment #368015 - Attachment is obsolete: true
Attachment #368574 - Flags: review?(sdwilsh)
Attached patch patch 3 (obsolete) (deleted) — — Splinter Review 
This patch:
* re-uses ENUMERATE_WEAKARRAY in the ENUMERATE_OBSERVERS macro
* updates the patch to trunk
* adds nsNavHistoryExpire.cpp observer enumerations
* adds a page change enumeration I missed from nsNavHistory.h
Attachment #368574 - Attachment is obsolete: true
Attachment #368586 - Flags: review?(sdwilsh)
Attachment #368574 - Flags: review?(sdwilsh)
Attachment #368586 - Flags: review?(sdwilsh) → review-
Comment on attachment 368586 [details] [diff] [review]
patch 3

>+++ b/toolkit/components/places/src/nsPlacesDBFlush.js
> function nsPlacesDBFlush()
> {
>   this._prefs = Cc["@mozilla.org/preferences-service;1"].
>                 getService(Ci.nsIPrefBranch);
>+  this._bs = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>+             getService(Ci.nsINavBookmarksService);
I don't think you want to do this - the point of this change is to not init bookmarks until the last possible minute, right?  You should make it a smart getter further down in this method though.

>   //////////////////////////////////////////////////////////////////////////////
>+  //// nsINavHistoryObserver
>+
>+  // We only use history observer to start the timer
This comment was a bit confusing.  How about something like this:
We currently only use the history observer to know when the history service actually does new work.  At that point, we actually get initialized, where our timer to sync history is added.

>+  // Batch methods match bookmark observer
nit: These methods share the name of the ones on nsINavBookmarkObserver, so their implementation can be found above.

>   //////////////////////////////////////////////////////////////////////////////
>   //// nsISupports
> 
>   classDescription: "Used to synchronize the temporary and permanent tables of Places",
>   classID: Components.ID("c1751cfc-e8f1-4ade-b0bb-f74edfb8ef6a"),
>   contractID: "@mozilla.org/places/sync;1",
>+  _xpcom_categories: [
>+    { category: "bookmark-observers" },
>+    { category: "history-observers" },
>+  ],
Can you please add a comment here about how registering in these categories makes us get initialized when those listeners would be notified please.

>+++ b/toolkit/components/places/src/nsPlacesMacros.h
>+// Call a method on each observer in a category cache, then call the same
>+// method the observer array, but only if the element is non-null.
The element shouldn't ever be null AFAIK.  These aren't weak pointers like the observer arrays are

>+#define ENUMERATE_OBSERVERS(cache, array, type, method)                        \
>+  {                                                                            \
>+    const nsCOMArray<type> &entries = cache.GetEntries();                      \
>+    for (PRInt32 entries_idx=0; entries_idx<entries.Count(); ++entries_idx) {  \
nit: spaces around =, <

>+      const nsCOMPtr<type> &e = entries[entries_idx];                          \
>+      if (e)                                                                   \
>+        e->method;                                                             \
So you should just be able to do entries[entries_idx]->method

Also, please use PR_[BEGIN|END]_MACRO, which should fix up some of your variable name issues that I'm guessing you ran into.

The worst part is that I'd like you to write a test for this.  I kinda want to see one for every possible listener notification, but that'd be an insane amount of test code.  I'll be happy with just making sure adding a bookmark, and adding a visit would notify something that was registered with the category manager.
Also - I count 39 calls to ENUMERATE_WEAKARRAY, and 39 calls to ENUMERATE_OBSERVERS with the current patch applied, so you got all the call sites.
Attached patch patch 4 (obsolete) (deleted) — — Splinter Review 
This patch is the same as the previous patch, but is adds a two unit tests which make sure a XPCOM object registered to "history-observers" or "bookmark-observers" will be created and loaded as needed.

The other change here is "mCanFireCache" in nsNavBookmarks.cpp which is used to as a flag for any ENUMERATE_ that can happen while the bookmarks service is initializing. Normal observers are still notified, but category observers are not. Testing showed it is very easy for a category observer to cause "Recursive GetService" calls while the bookmark service is initializing.
Attachment #368586 - Attachment is obsolete: true
Attachment #368970 - Flags: review?(sdwilsh)
In the tests, can you also make sure that the observer was notified (in addition to being created) please?
Attachment #368970 - Flags: review?(sdwilsh) → review-
Comment on attachment 368970 [details] [diff] [review]
patch 4

r- per comment 10 and discussion on irc
Attached patch patch 5 (obsolete) (deleted) — — Splinter Review 
Same as the previous patch. This patch:
* adds tests to make sure the dummy observer is notified as well as created
* changes the macro to except a flag to enable/disable the firing of notifications
* added a flag to nsNavHistory.cpp to control firing notifications - defaults to true
Attachment #368970 - Attachment is obsolete: true
Attachment #369315 - Flags: review?(sdwilsh)
Comment on attachment 369315 [details] [diff] [review]
patch 5

s/dummy-observer-added/dummy-observer-item-added/g

r=sdwilsh with that change
Attachment #369315 - Flags: review?(sdwilsh) → review+
Attached patch export patch with user and commit message (obsolete) (deleted) — — Splinter Review 
Changes made. Ready for commit.

This exported patch has the user and commit message in it
Attached patch unbitrotted export for checkin (obsolete) (deleted) — — Splinter Review 
Unbitrotted and I dropped the whitespace changes since they were the cause of the bitrot. Now less can go wrong (I hope)
Attachment #369353 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/2d1f7c7c7a2b
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Flags: in-litmus-
Keywords: checkin-needed
OS: Windows XP → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
Had to back this out due to TUnit failures at least on OS X and Windows.
http://hg.mozilla.org/mozilla-central/rev/cc7213e51266
http://hg.mozilla.org/mozilla-central/rev/be3109bef122
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Are there any changes that would prevent a page from being removed right away?

The failure is at..
Looking for http://foo.bar/tag/2, foo.bar  foo.bar in expected results...

Where that particular page (created with addPageBook(11, 1, 1, [1]); // title and url) has its visit removed by removePages([4,6,7,8,9,11]);

But some reason the autocomplete is still able to find page #11.

Does the test fail locally? If so, could you try modifying the test to run slightly later, if it passes, then perhaps we need to wait for the page's visit to actually get removed, if it's delayed somehow... but I don't think we delay removing pages... ?
Hi guys!

Mark asked me if I had some time to take a look at what's happening here with the unit tests failure., which I did this morning. I applied patch5, rebuilt and ran the unit tests.

first thought:
 Am I wrong, or the errors reported in comment #18 do not pertain to test_bookmark_catobs.js  or  test_history_catobs.js at all ? From what I noticed by looking at the the reported build error summary from comment #18, only head_autocomplete.js and test_special_search.js are in cause.

but:
now, what I noticed by running the unit tests under linux desktop is that test_history_catobs.js and test_bookmark_catobs.js were indeed failing on my box, but with those messages :

TEST-UNEXPECTED-FAIL | /<path_to_objdir>/xulrunner/_tests/xpcshell/test_places/unit/test_history_catobs.js | test failed, see following log:
  >>>>>>>
  *** test pending
/<path_to_objdir>/xulrunner/_tests/xpcshell/test_places/unit/toolkit/components/places/tests/unit/nsDummyObserver.js doesn't exist
[...]
<<<<<<<

the attached patch is very simple and fix relative paths used to load nsDummyObserver.js. After applying this patch, both unit tests passed without problems with the following output :


"TEST-PASS | /<path_to_objdir>/xulrunner/_tests/xpcshell/test_places/unit/test_history_catobs.js | all tests passed"

"TEST-PASS | /<path_to_objdir>/xulrunner/_tests/xpcshell/test_places/unit/test_bookmark_catobs.js | all tests passed"

... but be aware that the head_autocomplete and test_special_search were still failing... 

I'm not sure what's the relation between Finkle's "category-observers-tests" and the failure of head_autocomplete...
(In reply to comment #20)
> Am I wrong, or the errors reported in comment #18 do not pertain to
> test_bookmark_catobs.js  or  test_history_catobs.js at all ? From what I
> noticed by looking at the the reported build error summary from comment #18,
> only head_autocomplete.js and test_special_search.js are in cause.

Correct

> now, what I noticed by running the unit tests under linux desktop is that
> test_history_catobs.js and test_bookmark_catobs.js were indeed failing on my
> box, but with those messages :

snip

> the attached patch is very simple and fix relative paths used to load
> nsDummyObserver.js. After applying this patch, both unit tests passed without
> problems with the following output :

Funny thing, I tried that initially, but the test were failing on my Windows desktop builds. I'll test them again. Your fix, using relative paths, is preferred IMO.

> ... but be aware that the head_autocomplete and test_special_search were still
> failing... 
> 
> I'm not sure what's the relation between Finkle's "category-observers-tests"
> and the failure of head_autocomplete...

That is what we need to find out.

(In reply to comment #19)
> Are there any changes that would prevent a page from being removed right away?

There shouldn't be any delays with a page being removed. There might be a delay instantiating any category-based observers, since we delay that until the last possible moment before a notification needs to be fired.

> 
> The failure is at..
> Looking for http://foo.bar/tag/2, foo.bar  foo.bar in expected results...
> 
> Where that particular page (created with addPageBook(11, 1, 1, [1]); // title
> and url) has its visit removed by removePages([4,6,7,8,9,11]);
> 
> But some reason the autocomplete is still able to find page #11.
> 
> Does the test fail locally?
Yes.

> If so, could you try modifying the test to run
> slightly later, if it passes, then perhaps we need to wait for the page's visit
> to actually get removed, if it's delayed somehow... but I don't think we delay
> removing pages... ?

We can try modifying the test to make it pass.
Look what I've found :

Linux box
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1238183379.1238191576.14486.gz

OS X box
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1238183384.1238188518.5455.gz

WinNT box
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1238183387.1238192836.18043.gz

seems that the "nsDummyObserver.js : file not found" error did happen during the breakage, but on the Linux box only, as it was the case for me. mhh...
I'm looking into the autocomplete failures now.
I have some bad news too - this pretty much makes us leak anything that registers with the category manger as far as I can tell....
I'm at a bit of a loss for this failure right now.  I'll keep digging tomorrow.

As for the leak, we probably need to release the category observers on xpcom-shutdown, otherwise cycles could exist between the observer and the services.
Does this mean we have Cycle Collector coverage bugs (failure to scan/unlink certain XPCOM object graph edges)?

/be
(In reply to comment #26)
> Does this mean we have Cycle Collector coverage bugs (failure to scan/unlink
> certain XPCOM object graph edges)?
Probably not - nsNavBookmarks and nsNavHistory do not participate in cycle collection.  They could probably, but I thought we didn't want to make things participate in cycle collection unless it was hard to not free up cycles?
This bug is evidence that it's hard enough.

/be
(In reply to comment #28)
> This bug is evidence that it's hard enough.
Not sure I agree with that.  I bet mfinkle didn't even know he was leaking because leak testing was only recently turned on for xpcshell tests.  Places is pretty good about releasing everything it holds a reference to on shutdown, so I don't see how this is any different.  I just missed this in my review.

Shame, shame on me.
The decision to implement CC was not made only to handle a few cases. XPCOM has a design flaw, same as MSCOM, in failing to deal with cycles other than by leaking them, or burdening the programmer with implemeting OOB cycle-breaking or full GC. End of story!

If we don't use CC we will continue to have such bugs, and we will not catch them all before any given release point (and we generally don't fix them in the point releases). These leaks blight our rep, as the (historically worse) leaks in IE due to MSCOM+JScript blight IE's rep. We need to work smarter, not harder.

/be
I agree with that: CC is so that we don't have to reason successfully about every possible reference cycle, because we don't and can't in the large.  We didn't and couldn't here, not because you are incompetent but because you are human and our systems are lolomgwtf complex in their ownership semantics and lifecycles.

Embrace the machine assistance.
So, a couple of things.  nsNavHistory is allowed to be accessed on multiple threads (namely, so folks can get the db connection on multiple threads), which means no cycle collection, right?

Also, I'm not sure why the category cache isn't already being cleared given the fact that nsCategoryObserver registers for xpcom-shutdown and clears itself anyway.
Attached patch v6.0 (obsolete) (deleted) — — Splinter Review 
Updated patch
Attachment #369315 - Attachment is obsolete: true
Attachment #369692 - Attachment is obsolete: true
Attachment #373134 - Attachment is obsolete: true
(In reply to comment #32)
> So, a couple of things.  nsNavHistory is allowed to be accessed on multiple
> threads (namely, so folks can get the db connection on multiple threads), which
> means no cycle collection, right?

Does anyone use nsNavHistory from more than one thread? How's that working out?

XPCOM thread safety is a bit of a myth, which is not to say a falsehood. You may be lucky; you may be on old, stable code. If not, watch out. Is this truly the important use-case to justify hand-management of cycles?

/be
what is the effect of this on Fennec?
(In reply to comment #35)
> what is the effect of this on Fennec?

The nsPlacesDBFlush init at startup is approx ~270ms and after startup we have a ~60-70ms hit during code that fires in a nsITimer
tracking-fennec: ? → 1.0b3+
Nominating this for blocking 1.9.1 due to comment 36 and that this is blocking the release of Fennec.  Drivers should feel free to bug me or mfinkle if they have any questions.
Flags: wanted1.9.2?
Flags: blocking1.9.1?
I'd approve a patch here, for sure, but not sure I can block the release on this. Would definitely appreciate your continued efforts here, though.
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
autocomplete tests, and Places unit tests in general, were not initializing nsPlacesDBFlush at all, we had to start it explicitely, that's because xpcshell does not fire profile-after-change. They were working with data in temp tables mostly, and we were activating flush manually only for sync tests.

With these changes all unit tests will run with sync enabled, that's an interesting change since iirc some test was starting sync later and relying on that. Also, that could explain some of the failures.
I'm trying to put togheter a patch that should solve tests leaks, most likely there will be still some failure.
so i see the following failures:
autocomplete/test_empty_search.js fail
autocomplete/test_special_search.js fail
unit/test_404630.js leak
unit/test_000_frecency.js endless loop
unit/test_384228.js children are randonly returned in wrong order (bad position value in db)
and failure in browser/components/places/tests/unit/test_placesTxn.js
Attached patch v6.1 (obsolete) (deleted) — — Splinter Review 
unbitrot, fix leaks, finalize missing statements in tests.
This has the above failures, posting here so we can maybe work in parallel on those failures.
Attachment #373668 - Attachment is obsolete: true
so, i've probably solved failure in unit/test_384228.js, since we have temp tables the order of bookmarks could vary due to status of temp and disk tables, this looks like a real Places bug, we should probably sort by ROWID if this is a bookmarks query.
or better by b.id ASC
Attached patch v6.2 (obsolete) (deleted) — — Splinter Review 
fixed a problem in an autocomplete query that was filtering out a perfect valid result due to duplicates with an inner limit.

i can see all unit tests pass. i'll push to the tryserver to check if i missed something.
Attachment #377675 - Attachment is obsolete: true
from Talos i don't see a gain for us.

about tests all 3 unit tests failed, but failures don't look related:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1242745835.1242754636.19969.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1242745136.1242750928.10604.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1242747760.1242755709.22297.gz

Hard to tell with all of those orange randoms.
Comment on attachment 378328 [details] [diff] [review]
v6.2

asking review on my changes.

I'll push again to the tryserver since i think most of the above errors (if not all) are due to their overload.
Attachment #378328 - Flags: review?(sdwilsh)
now i see a green, a random orange not related and a strange leak, practically all unit tests (not only places ones) do leak nsStringBuffers.
No idea about this new leak.
This is the log:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1242783346.1242790447.32009.gz
i'm splitting out Places bugs i've found to their own bugs, hoping to be able to take them before the freeze
filed bug 493933 and bug 493934, added dependancies.
Depends on: 493933, 493934
(In reply to comment #52)
> filed bug 493933 and bug 493934, added dependancies.
Does that mean this patch isn't totally valid anymore, or did you make this patch on top of those?
needs an unbitrot, removing those changes mainly.
Attached patch patch v6.3 (obsolete) (deleted) — — Splinter Review 
unbitrot v6.2. Still the nsStringBuffer leak needs to be investigated.
Attachment #378328 - Attachment is obsolete: true
Attachment #379167 - Flags: review?(sdwilsh)
Attachment #378328 - Flags: review?(sdwilsh)
Attached patch v6.3 review comments (in diff form) (obsolete) (deleted) — — Splinter Review 
Have not yet tackled the leak yet.
Attachment #379969 - Flags: review?(mak77)
Comment on attachment 379167 [details] [diff] [review]
patch v6.3

r+ with the review comments I just addressed.
Attachment #379167 - Flags: review?(sdwilsh) → review+
Comment on attachment 379969 [details] [diff] [review]
v6.3 review comments (in diff form)

>+/**
>+ * Flushes any events in the event loop of the main thread.
>+ */
>+function flush_events()

i would prefer flush_mainThread_events, it's not clear which events this function is flushing when you see it in tails.
Attachment #379969 - Flags: review?(mak77) → review+
I'll own this bug for the foreseeable future.
Assignee: mark.finkle → sdwilsh
Status: REOPENED → ASSIGNED
Attached patch v6.4 (obsolete) (deleted) — — Splinter Review 
This is my review comments folded into the patch.  This doesn't leak locally, so I've pushed this to the try server to verify this.  In the mean time, we'll have dietrich take a look at this to make sure mak and I haven't gone insane.
Attachment #379167 - Attachment is obsolete: true
Attachment #379969 - Attachment is obsolete: true
Attachment #379991 - Flags: review?(dietrich)
Whiteboard: [needs review dietrich]
Attached patch v6.5 (obsolete) (deleted) — — Splinter Review 
forgot the changes to browser/
Attachment #379991 - Attachment is obsolete: true
Attachment #379994 - Flags: review?(dietrich)
Attachment #379991 - Flags: review?(dietrich)
I looked at trybuilds near my push on 19/05 and they had the same nsStringBuffer leak, so it's probably not related to this patch. Btw let's wait for new trybuilds results.
We are still leaking 3 string buffers on all three platforms on xpcshell tests (but is likely a test issue since other test harnesses aren't reporting this leak).  This is odd because I was not seeing this locally.  Windows also hung on unit tests - this is the last test that ran:
unit\test_placeURIs.js
These leaks that are happening on the try sever are also happening on tinderbox, but aren't locally reproducible.  This is really weird, but I'm sure it's not this patch's fault anymore.
Comment on attachment 379994 [details] [diff] [review]
v6.5


>@@ -88,7 +89,7 @@ nsNavBookmarks* nsNavBookmarks::sInstanc
> 
> nsNavBookmarks::nsNavBookmarks()
>   : mItemCount(0), mRoot(0), mBookmarksRoot(0), mTagRoot(0), mToolbarFolder(0), mBatchLevel(0),
>-    mBatchHasTransaction(PR_FALSE)
>+    mBatchHasTransaction(PR_FALSE), mCanFireNotifs(false), mCacheObservers("bookmark-observers")
> {

i'd prefer mCanNotify or mCanFireNotifications

whole words ftw

>@@ -278,8 +279,8 @@ nsNavHistoryExpire::ClearHistory()
>   // forcibly call the "on idle" timer here to do a little work
>   // but the rest will happen on idle.
> 
>-  ENUMERATE_WEAKARRAY(mHistory->mObservers, nsINavHistoryObserver,
>-                      OnClearHistory())
>+  ENUMERATE_OBSERVERS(true, mHistory->mCacheObservers, mHistory->mObservers,
>+                      nsINavHistoryObserver, OnClearHistory())
> 
>   return NS_OK;
> }

why forcing true instead of using the nsNavHistory property?

>@@ -383,7 +384,8 @@ nsNavHistoryExpire::ExpireItems(PRUint32
>     // FIXME bug 325241 provide a way to observe hidden elements
>     if (expiredVisits[i].hidden) continue;
> 
>-    ENUMERATE_WEAKARRAY(mHistory->mObservers, nsINavHistoryObserver,
>+    ENUMERATE_OBSERVERS(true, mHistory->mCacheObservers, mHistory->mObservers,
>+                        nsINavHistoryObserver,
>                         OnPageExpired(uri, expiredVisits[i].visitDate,
>                                       expiredVisits[i].erased));
>   }

ditto

>+#include "prtypes.h"
>+// Call a method on each observer in a category cache, then call the same
>+// method the observer array.

s/method the/method on the/

>diff --git a/toolkit/components/places/tests/bookmarks/test_384228.js b/toolkit/components/places/tests/bookmarks/test_384228.js
>--- a/toolkit/components/places/tests/bookmarks/test_384228.js
>+++ b/toolkit/components/places/tests/bookmarks/test_384228.js
>@@ -59,24 +59,34 @@ function run_test() {
>   // test querying for bookmarks in multiple folders
>   var testFolder1 = bmsvc.createFolder(root, "bug 384228 test folder 1",
>                                        bmsvc.DEFAULT_INDEX);
>+  do_check_eq(bmsvc.getItemIndex(testFolder1), 0);
>   var testFolder2 = bmsvc.createFolder(root, "bug 384228 test folder 2",
>                                        bmsvc.DEFAULT_INDEX);
>+  do_check_eq(bmsvc.getItemIndex(testFolder2), 1);
>   var testFolder3 = bmsvc.createFolder(root, "bug 384228 test folder 3",
>                                        bmsvc.DEFAULT_INDEX);
>+  do_check_eq(bmsvc.getItemIndex(testFolder3), 2);
> 
>   var b1 = bmsvc.insertBookmark(testFolder1, uri("http://foo.tld/"),
>                                 bmsvc.DEFAULT_INDEX, "title b1 (folder 1)");
>+  do_check_eq(bmsvc.getItemIndex(b1), 0);
>   var b2 = bmsvc.insertBookmark(testFolder1, uri("http://foo.tld/"),
>                                 bmsvc.DEFAULT_INDEX, "title b2 (folder 1)");
>+  do_check_eq(bmsvc.getItemIndex(b2), 1);
>   var b3 = bmsvc.insertBookmark(testFolder2, uri("http://foo.tld/"),
>                                 bmsvc.DEFAULT_INDEX, "title b3 (folder 2)");
>+  do_check_eq(bmsvc.getItemIndex(b3), 0);
>   var b4 = bmsvc.insertBookmark(testFolder3, uri("http://foo.tld/"),
>                                 bmsvc.DEFAULT_INDEX, "title b4 (folder 3)");
>+  do_check_eq(bmsvc.getItemIndex(b4), 0);
>   // also test recursive search
>   var testFolder1_1 = bmsvc.createFolder(testFolder1, "bug 384228 test folder 1.1",
>                                          bmsvc.DEFAULT_INDEX);
>+  do_check_eq(bmsvc.getItemIndex(testFolder1_1), 2);
>   var b5 = bmsvc.insertBookmark(testFolder1_1, uri("http://a1.com/"),
>                                 bmsvc.DEFAULT_INDEX, "title b5 (folder 1.1)");
>+  do_check_eq(bmsvc.getItemIndex(b5), 0);
>+

are these changes related to this patch somehow?

r=me otherwise
Attachment #379994 - Flags: review?(dietrich) → review+
Whiteboard: [needs review dietrich]
(In reply to comment #65)
> are these changes related to this patch somehow?
mak added that stuff while he was debugging the test failures in the first place.  It doesn't hurt to leave them in, so I left them in.
Attached patch v6.6 (obsolete) (deleted) — — Splinter Review 
Addresses review comments.
Attachment #379994 - Attachment is obsolete: true
Whiteboard: [can land]
Attached patch branch patch v1.0 (obsolete) (deleted) — — Splinter Review 
I had to get a bit clever in nsPlacesMacros.h because of our branch-only interfaces.  I didn't want to add another parameter to our macro, so I added some templated structs that the macro uses.
This was the only major change, and everything else was just some minor differences between branch and trunk.

This passes tests locally.
Attachment #380514 - Flags: review?(dietrich)
Comment on attachment 380514 [details] [diff] [review]
branch patch v1.0

># HG changeset patch
># User Mark Finkle <mfinkle@mozilla.com>, Shawn Wilsher <me@shawnwilsher.com>, Marco Bonardo <mak77@bonardo.net>
># Date 1238013096 18000
># Node ID d56d534624c508bcab48d8d9c9d44e07150e7939
># Parent  7509164476e804fcf9deb29e76075ee68625ce35
>Bug 483980 - Allow history/bookmark observer components to register with a startup category
>This changeset allows for history and bookmark observers to be registered via
>the category manager.  This means the bookmarks service does not have to be
>initialized at startup to register observers with it, as well as the history
>service.
>r=sdwilsh
>r=mak
>r=dietrich
>
>diff --git a/browser/components/places/tests/unit/head_bookmarks.js b/browser/components/places/tests/unit/head_bookmarks.js
>--- a/browser/components/places/tests/unit/head_bookmarks.js
>+++ b/browser/components/places/tests/unit/head_bookmarks.js
>@@ -327,3 +327,13 @@ function dump_table(aName)
>   stmt.finalize();
>   stmt = null;
> }
>+
>+/**
>+ * Flushes any events in the event loop of the main thread.
>+ */
>+function flush_main_thread_events()
>+{
>+  let tm = Cc["@mozilla.org/thread-manager;1"].getService(Ci.nsIThreadManager);
>+  while (tm.mainThread.hasPendingEvents())
>+    tm.mainThread.processNextEvent(false);
>+}
>diff --git a/browser/components/places/tests/unit/tail_bookmarks.js b/browser/components/places/tests/unit/tail_bookmarks.js
>--- a/browser/components/places/tests/unit/tail_bookmarks.js
>+++ b/browser/components/places/tests/unit/tail_bookmarks.js
>@@ -41,9 +41,7 @@
> // event loop long before code like this would run.
> // Not doing so could cause us to close the connection between all tasks have
> // been completed, and that would crash badly.
>-let tm = Cc["@mozilla.org/thread-manager;1"].getService(Ci.nsIThreadManager);
>-while (tm.mainThread.hasPendingEvents())
>-  tm.mainThread.processNextEvent(false);
>+flush_main_thread_events();
> 
> // XPCShell doesn't dispatch quit-application, to ensure cleanup we have to
> // dispatch it after each test run.
>@@ -52,6 +50,9 @@ var os = Cc['@mozilla.org/observer-servi
> os.notifyObservers(null, "quit-application-granted", null);
> os.notifyObservers(null, "quit-application", null);
> 
>+// Run the event loop, since we enqueue some statement finalization.
>+flush_main_thread_events();
>+
> // try to close the connection so we can remove places.sqlite
> var pip = Cc["@mozilla.org/browser/nav-history-service;1"].
>           getService(Ci.nsINavHistoryService).
>diff --git a/toolkit/components/places/src/nsNavBookmarks.cpp b/toolkit/components/places/src/nsNavBookmarks.cpp
>--- a/toolkit/components/places/src/nsNavBookmarks.cpp
>+++ b/toolkit/components/places/src/nsNavBookmarks.cpp
>@@ -54,6 +54,7 @@
> #include "nsPlacesTriggers.h"
> #include "nsPlacesTables.h"
> #include "nsPlacesIndexes.h"
>+#include "nsPlacesMacros.h"
> 
> const PRInt32 nsNavBookmarks::kFindBookmarksIndex_ID = 0;
> const PRInt32 nsNavBookmarks::kFindBookmarksIndex_Type = 1;
>@@ -88,7 +89,7 @@ nsNavBookmarks* nsNavBookmarks::sInstanc
> 
> nsNavBookmarks::nsNavBookmarks()
>   : mItemCount(0), mRoot(0), mBookmarksRoot(0), mTagRoot(0), mToolbarFolder(0), mBatchLevel(0),
>-    mBatchHasTransaction(PR_FALSE)
>+    mBatchHasTransaction(PR_FALSE), mCanNotify(false), mCacheObservers("bookmark-observers")
> {
>   NS_ASSERTION(!sInstance, "Multiple nsNavBookmarks instances!");
>   sInstance = this;
>@@ -125,6 +126,8 @@ nsNavBookmarks::Init()
>   rv = transaction.Commit();
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  mCanNotify = true;
>+
>   // Add observers
>   nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService();
>   NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY);
>@@ -1109,7 +1112,7 @@ nsNavBookmarks::InsertBookmark(PRInt64 a
> 
>   AddBookmarkToHash(childID, 0);
> 
>-  ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                       OnItemAdded(rowId, aFolder, index))
> 
>   // If the bookmark has been added to a tag container, notify all
>@@ -1127,7 +1130,7 @@ nsNavBookmarks::InsertBookmark(PRInt64 a
> 
>     if (bookmarks.Length()) {
>       for (PRUint32 i = 0; i < bookmarks.Length(); i++) {
>-        ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+        ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                             OnItemChanged(bookmarks[i], NS_LITERAL_CSTRING("tags"),
>                                           PR_FALSE, EmptyCString()))
>       }
>@@ -1175,7 +1178,7 @@ nsNavBookmarks::RemoveItem(PRInt64 aItem
>     return NS_OK;
>   }
> 
>-  ENUMERATE_WEAKARRAY(mObservers,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
>                       nsINavBookmarkObserver_MOZILLA_1_9_1_ADDITIONS,
>                       OnBeforeItemRemoved(aItemId))
> 
>@@ -1215,7 +1218,7 @@ nsNavBookmarks::RemoveItem(PRInt64 aItem
>     NS_ENSURE_SUCCESS(rv, rv);
>   }
> 
>-  ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                       OnItemRemoved(aItemId, folderId, childIndex))
> 
>   if (itemType == TYPE_BOOKMARK) {
>@@ -1236,7 +1239,7 @@ nsNavBookmarks::RemoveItem(PRInt64 aItem
> 
>       if (bookmarks.Length()) {
>         for (PRUint32 i = 0; i < bookmarks.Length(); i++) {
>-          ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+          ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                               OnItemChanged(bookmarks[i], NS_LITERAL_CSTRING("tags"),
>                                             PR_FALSE, EmptyCString()))
>         }
>@@ -1385,7 +1388,7 @@ nsNavBookmarks::CreateContainerWithID(PR
>   rv = transaction.Commit();
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                       OnItemAdded(id, aParent, index))
> 
>   *aIndex = index;
>@@ -1450,7 +1453,7 @@ nsNavBookmarks::InsertSeparator(PRInt64 
>   rv = transaction.Commit();
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                       OnItemAdded(rowId, aParent, index))
> 
>   return NS_OK;
>@@ -1587,7 +1590,7 @@ nsNavBookmarks::RemoveFolder(PRInt64 aFo
> {
>   NS_ENSURE_TRUE(aFolderId != mRoot, NS_ERROR_INVALID_ARG);
> 
>-  ENUMERATE_WEAKARRAY(mObservers,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
>                       nsINavBookmarkObserver_MOZILLA_1_9_1_ADDITIONS,
>                       OnBeforeItemRemoved(aFolderId))
> 
>@@ -1662,7 +1665,7 @@ nsNavBookmarks::RemoveFolder(PRInt64 aFo
>     mToolbarFolder = 0;
>   }
> 
>-  ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                       OnItemRemoved(aFolderId, parent, index))
> 
>   return NS_OK;
>@@ -1780,7 +1783,7 @@ nsNavBookmarks::RemoveFolderChildren(PRI
>     folderChildrenInfo child = folderChildrenArray[i];
> 
>     // Notify observers that we are about to remove this child.
>-    ENUMERATE_WEAKARRAY(mObservers,
>+    ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
>                         nsINavBookmarkObserver_MOZILLA_1_9_1_ADDITIONS,
>                         OnBeforeItemRemoved(child.itemId))
> 
>@@ -1854,7 +1857,7 @@ nsNavBookmarks::RemoveFolderChildren(PRI
>   for (PRInt32 i = folderChildrenArray.Length() - 1; i >= 0 ; i--) {
>     folderChildrenInfo child = folderChildrenArray[i];
> 
>-    ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+    ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                         OnItemRemoved(child.itemId,
>                                       child.parentId,
>                                       child.index));
>@@ -1875,7 +1878,7 @@ nsNavBookmarks::RemoveFolderChildren(PRI
> 
>         if (bookmarks.Length()) {
>           for (PRUint32 i = 0; i < bookmarks.Length(); i++) {
>-            ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+            ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                                 OnItemChanged(bookmarks[i], NS_LITERAL_CSTRING("tags"),
>                                               PR_FALSE, EmptyCString()))
>           }
>@@ -2032,7 +2035,7 @@ nsNavBookmarks::MoveItem(PRInt64 aItemId
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // notify bookmark observers
>-  ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                       OnItemMoved(aItemId, oldParent, oldIndex, aNewParent,
>                                   newIndex))
> 
>@@ -2107,7 +2110,7 @@ nsNavBookmarks::SetItemDateAdded(PRInt64
>   nsresult rv = SetItemDateInternal(mDBSetItemDateAdded, aItemId, aDateAdded);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                       OnItemChanged(aItemId, NS_LITERAL_CSTRING("dateAdded"),
>                                     PR_FALSE, nsPrintfCString(16, "%lld", aDateAdded)));
>   return NS_OK;
>@@ -2138,7 +2141,7 @@ nsNavBookmarks::SetItemLastModified(PRIn
>   nsresult rv = SetItemDateInternal(mDBSetItemLastModified, aItemId, aLastModified);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                       OnItemChanged(aItemId, NS_LITERAL_CSTRING("lastModified"),
>                                     PR_FALSE, nsPrintfCString(16, "%lld", aLastModified)));
>   return NS_OK;
>@@ -2261,7 +2264,7 @@ nsNavBookmarks::SetItemTitle(PRInt64 aIt
>   rv = statement->Execute();
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                       OnItemChanged(aItemId, NS_LITERAL_CSTRING("title"),
>                                     PR_FALSE, aTitle));
>   return NS_OK;
>@@ -2639,7 +2642,7 @@ nsNavBookmarks::ChangeBookmarkURI(PRInt6
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // Pass the new URI to OnItemChanged.
>-  ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>     OnItemChanged(aBookmarkId, NS_LITERAL_CSTRING("uri"), PR_FALSE, spec))
> 
>   return NS_OK;
>@@ -2766,12 +2769,14 @@ nsNavBookmarks::SetItemIndex(PRInt64 aIt
> 
>   // XXX (bug 484096) this is really inefficient and we should look into using
>   //     onItemChanged here!
>-  ENUMERATE_WEAKARRAY(mObservers,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
>                       nsINavBookmarkObserver_MOZILLA_1_9_1_ADDITIONS,
>                       OnBeforeItemRemoved(aItemId))
>-  ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
>+                      nsINavBookmarkObserver,
>                       OnItemRemoved(aItemId, parent, oldIndex))
>-  ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
>+                      nsINavBookmarkObserver,
>                       OnItemAdded(aItemId, parent, aNewIndex))
> 
>   return NS_OK;
>@@ -2857,7 +2862,7 @@ nsNavBookmarks::SetKeywordForBookmark(PR
>   transaction.Commit();
> 
>   // Pass the new keyword to OnItemChanged.
>-  ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                       OnItemChanged(aBookmarkId, NS_LITERAL_CSTRING("keyword"),
>                                     PR_FALSE, NS_ConvertUTF16toUTF8(aKeyword)))
> 
>@@ -2943,7 +2948,7 @@ nsNavBookmarks::BeginUpdateBatch()
>     if (mBatchHasTransaction)
>       conn->BeginTransaction();
> 
>-    ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+    ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                         OnBeginUpdateBatch())
>   }
>   return NS_OK;
>@@ -2956,7 +2961,7 @@ nsNavBookmarks::EndUpdateBatch()
>     if (mBatchHasTransaction)
>       mDBConn->CommitTransaction();
>     mBatchHasTransaction = PR_FALSE;
>-    ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+    ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                         OnEndUpdateBatch())
>   }
>   return NS_OK;
>@@ -3029,7 +3034,7 @@ nsNavBookmarks::OnVisit(nsIURI *aURI, PR
> 
>     if (bookmarks.Length()) {
>       for (PRUint32 i = 0; i < bookmarks.Length(); i++)
>-        ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+        ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                             OnItemVisited(bookmarks[i], aVisitID, aTime))
>     }
>   }
>@@ -3051,7 +3056,7 @@ nsNavBookmarks::OnDeleteURI(nsIURI *aURI
> 
>     if (bookmarks.Length()) {
>       for (PRUint32 i = 0; i < bookmarks.Length(); i ++)
>-        ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+        ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                             OnItemChanged(bookmarks[i], NS_LITERAL_CSTRING("cleartime"),
>                                           PR_FALSE, EmptyCString()))
>     }
>@@ -3100,7 +3105,7 @@ nsNavBookmarks::OnPageChanged(nsIURI *aU
>       NS_ENSURE_STATE(queries.Count() == 1);
>       NS_ENSURE_STATE(queries[0]->Folders().Length() == 1);
> 
>-      ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+      ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                           OnItemChanged(queries[0]->Folders()[0], NS_LITERAL_CSTRING("favicon"),
>                                         PR_FALSE, NS_ConvertUTF16toUTF8(aValue)));
>     }
>@@ -3112,7 +3117,7 @@ nsNavBookmarks::OnPageChanged(nsIURI *aU
> 
>       if (bookmarks.Length()) {
>         for (PRUint32 i = 0; i < bookmarks.Length(); i ++)
>-          ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+          ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                               OnItemChanged(bookmarks[i], NS_LITERAL_CSTRING("favicon"),
>                                             PR_FALSE, NS_ConvertUTF16toUTF8(aValue)));
>       }
>@@ -3143,7 +3148,7 @@ nsNavBookmarks::OnItemAnnotationSet(PRIn
>   nsresult rv = SetItemDateInternal(mDBSetItemLastModified, aItemId, PR_Now());
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                       OnItemChanged(aItemId, aName, PR_TRUE, EmptyCString()));
> 
>   return NS_OK;
>@@ -3162,7 +3167,7 @@ nsNavBookmarks::OnItemAnnotationRemoved(
>   nsresult rv = SetItemDateInternal(mDBSetItemLastModified, aItemId, PR_Now());
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                       OnItemChanged(aItemId, aName, PR_TRUE, EmptyCString()));
> 
>   return NS_OK;
>diff --git a/toolkit/components/places/src/nsNavBookmarks.h b/toolkit/components/places/src/nsNavBookmarks.h
>--- a/toolkit/components/places/src/nsNavBookmarks.h
>+++ b/toolkit/components/places/src/nsNavBookmarks.h
>@@ -45,6 +45,7 @@
> #include "nsNavHistory.h"
> #include "nsNavHistoryResult.h" // need for Int64 hashtable
> #include "nsToolkitCompsCID.h"
>+#include "nsCategoryCache.h"
> 
> class nsIOutputStream;
> 
>@@ -296,6 +297,10 @@ private:
>     nsString mType;
>     PRInt32 mIndex;
>   };
>+
>+  // Used to enable and disable the observer notifications.
>+  bool mCanNotify;
>+  nsCategoryCache<nsINavBookmarkObserver> mCacheObservers;
> };
> 
> struct nsBookmarksUpdateBatcher
>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp
>--- a/toolkit/components/places/src/nsNavHistory.cpp
>+++ b/toolkit/components/places/src/nsNavHistory.cpp
>@@ -52,6 +52,7 @@
> #include "nsPlacesTables.h"
> #include "nsPlacesIndexes.h"
> #include "nsPlacesTriggers.h"
>+#include "nsPlacesMacros.h"
> 
> #include "nsIArray.h"
> #include "nsArrayEnumerator.h"
>@@ -446,7 +447,9 @@ nsNavHistory::nsNavHistory() : mBatchLev
>                                mNumVisitsForFrecency(10),
>                                mTagsFolder(-1),
>                                mInPrivateBrowsing(PRIVATEBROWSING_NOTINITED),
>-                               mDatabaseStatus(DATABASE_STATUS_OK)
>+                               mDatabaseStatus(DATABASE_STATUS_OK),
>+                               mCanNotify(true),
>+                               mCacheObservers("history-observers")
> {
> #ifdef LAZY_ADD
>   mLazyTimerSet = PR_TRUE;
>@@ -2892,7 +2895,7 @@ nsNavHistory::AddVisit(nsIURI* aURI, PRT
>   PRUint32 added = 0;
>   if (!hidden && aTransitionType != TRANSITION_EMBED &&
>                  aTransitionType != TRANSITION_DOWNLOAD) {
>-    ENUMERATE_WEAKARRAY(mObservers, nsINavHistoryObserver,
>+    ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavHistoryObserver,
>                         OnVisit(aURI, *aVisitID, aTime, aSessionID,
>                                 referringVisitID, aTransitionType, &added));
>   }
>@@ -4197,7 +4200,7 @@ nsNavHistory::BeginUpdateBatch()
>     if (mBatchHasTransaction)
>       mDBConn->BeginTransaction();
> 
>-    ENUMERATE_WEAKARRAY(mObservers, nsINavHistoryObserver,
>+    ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavHistoryObserver,
>                         OnBeginUpdateBatch())
>   }
>   return NS_OK;
>@@ -4211,7 +4214,8 @@ nsNavHistory::EndUpdateBatch()
>     if (mBatchHasTransaction)
>       mDBConn->CommitTransaction();
>     mBatchHasTransaction = PR_FALSE;
>-    ENUMERATE_WEAKARRAY(mObservers, nsINavHistoryObserver, OnEndUpdateBatch())
>+    ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavHistoryObserver,
>+                        OnEndUpdateBatch())
>   }
>   return NS_OK;
> }
>@@ -4522,7 +4526,8 @@ nsNavHistory::RemovePage(nsIURI *aURI)
>   NS_ENSURE_ARG(aURI);
> 
>   // Before we remove, we have to notify our observers!
>-  ENUMERATE_WEAKARRAY(mObservers, nsINavHistoryObserver_MOZILLA_1_9_1_ADDITIONS,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
>+                      nsINavHistoryObserver_MOZILLA_1_9_1_ADDITIONS,
>                       OnBeforeDeleteURI(aURI))
> 
>   nsIURI** URIs = &aURI;
>@@ -4530,7 +4535,8 @@ nsNavHistory::RemovePage(nsIURI *aURI)
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // Notify our observers that the URI has been removed.
>-  ENUMERATE_WEAKARRAY(mObservers, nsINavHistoryObserver, OnDeleteURI(aURI))
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
>+                      nsINavHistoryObserver, OnDeleteURI(aURI))
>   return NS_OK;
> }
> 
>@@ -4890,7 +4896,7 @@ nsNavHistory::HidePage(nsIURI *aURI)
> 
>   // notify observers, finish transaction first
>   transaction.Commit();
>-  ENUMERATE_WEAKARRAY(mObservers, nsINavHistoryObserver,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavHistoryObserver,
>                       OnPageChanged(aURI,
>                                     nsINavHistoryObserver::ATTRIBUTE_HIDDEN,
>                                     EmptyString()))
>@@ -6745,6 +6751,14 @@ nsNavHistory::BookmarkIdToResultNode(PRI
>   return RowToResult(stmt, aOptions, aResult);
> }
> 
>+void
>+nsNavHistory::SendPageChangedNotification(nsIURI* aURI, PRUint32 aWhat,
>+                                          const nsAString& aValue)
>+{
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavHistoryObserver,
>+                      OnPageChanged(aURI, aWhat, aValue));
>+}
>+
> // nsNavHistory::TitleForDomain
> //
> //    This computes the title for a given domain. Normally, this is just the
>@@ -6874,11 +6888,10 @@ nsNavHistory::SetPageTitleInternal(nsIUR
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // observers (have to check first if it's bookmarked)
>-  ENUMERATE_WEAKARRAY(mObservers, nsINavHistoryObserver,
>+  ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavHistoryObserver,
>                       OnTitleChanged(aURI, aTitle))
> 
>   return NS_OK;
>-
> }
> 
> nsresult
>diff --git a/toolkit/components/places/src/nsNavHistory.h b/toolkit/components/places/src/nsNavHistory.h
>--- a/toolkit/components/places/src/nsNavHistory.h
>+++ b/toolkit/components/places/src/nsNavHistory.h
>@@ -82,6 +82,7 @@
> #include "nsTArray.h"
> #include "nsINavBookmarksService.h"
> #include "nsMaybeWeakPtr.h"
>+#include "nsCategoryCache.h"
> 
> #include "nsNavHistoryExpire.h"
> #include "nsNavHistoryResult.h"
>@@ -292,11 +293,7 @@ public:
>   // used by other places components to send history notifications (for example,
>   // when the favicon has changed)
>   void SendPageChangedNotification(nsIURI* aURI, PRUint32 aWhat,
>-                                   const nsAString& aValue)
>-  {
>-    ENUMERATE_WEAKARRAY(mObservers, nsINavHistoryObserver,
>-                        OnPageChanged(aURI, aWhat, aValue));
>-  }
>+                                   const nsAString& aValue);
> 
>   // current time optimization
>   PRTime GetNow();
>@@ -391,6 +388,12 @@ public:
>     return NS_OK;
>   }
> 
>+  /**
>+   * Indicates if it is OK to notify history observers or not.
>+   *
>+   * @returns true if it is OK to notify, false otherwise.
>+   */
>+  bool canNotify() { return mCanNotify; }
>  private:
>   ~nsNavHistory();
> 
>@@ -830,6 +833,10 @@ protected:
>   PRBool mInPrivateBrowsing;
> 
>   PRUint16 mDatabaseStatus;
>+
>+  // Used to enable and disable the observer notifications
>+  bool mCanNotify;
>+  nsCategoryCache<nsINavHistoryObserver> mCacheObservers;
> };
> 
> /**
>diff --git a/toolkit/components/places/src/nsNavHistoryExpire.cpp b/toolkit/components/places/src/nsNavHistoryExpire.cpp
>--- a/toolkit/components/places/src/nsNavHistoryExpire.cpp
>+++ b/toolkit/components/places/src/nsNavHistoryExpire.cpp
>@@ -47,6 +47,7 @@
> #include "nsNetUtil.h"
> #include "nsIAnnotationService.h"
> #include "nsPrintfCString.h"
>+#include "nsPlacesMacros.h"
> 
> struct nsNavHistoryExpireRecord {
>   nsNavHistoryExpireRecord(mozIStorageStatement* statement);
>@@ -287,7 +288,8 @@ nsNavHistoryExpire::ClearHistory()
>   // forcibly call the "on idle" timer here to do a little work
>   // but the rest will happen on idle.
> 
>-  ENUMERATE_WEAKARRAY(mHistory->mObservers, nsINavHistoryObserver,
>+  ENUMERATE_OBSERVERS(mHistory->canNotify(), mHistory->mCacheObservers,
>+                      mHistory->mObservers, nsINavHistoryObserver,
>                       OnClearHistory())
> 
>   return NS_OK;
>@@ -392,7 +394,8 @@ nsNavHistoryExpire::ExpireItems(PRUint32
>     // FIXME bug 325241 provide a way to observe hidden elements
>     if (expiredVisits[i].hidden) continue;
> 
>-    ENUMERATE_WEAKARRAY(mHistory->mObservers, nsINavHistoryObserver,
>+    ENUMERATE_OBSERVERS(mHistory->canNotify(), mHistory->mCacheObservers,
>+                        mHistory->mObservers, nsINavHistoryObserver,
>                         OnPageExpired(uri, expiredVisits[i].visitDate,
>                                       expiredVisits[i].erased));
>   }
>diff --git a/toolkit/components/places/src/nsPlacesDBFlush.js b/toolkit/components/places/src/nsPlacesDBFlush.js
>--- a/toolkit/components/places/src/nsPlacesDBFlush.js
>+++ b/toolkit/components/places/src/nsPlacesDBFlush.js
>@@ -77,10 +77,6 @@ function nsPlacesDBFlush()
>   }
> 
>   // Register observers
>-  this._bs = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>-             getService(Ci.nsINavBookmarksService);
>-  this._bs.addObserver(this, false);
>-
>   this._os = Cc["@mozilla.org/observer-service;1"].
>              getService(Ci.nsIObserverService);
>   this._os.addObserver(this, kQuitApplication, false);
>@@ -101,6 +97,12 @@ function nsPlacesDBFlush()
>                       DBConnection;
>   });
> 
>+  this.__defineGetter__("_bs", function() {
>+    delete this._bs;
>+    return this._bs = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>+                      getService(Ci.nsINavBookmarksService);
>+  });
>+
> }
> 
> nsPlacesDBFlush.prototype = {
>@@ -110,7 +112,6 @@ nsPlacesDBFlush.prototype = {
>   observe: function DBFlush_observe(aSubject, aTopic, aData)
>   {
>     if (aTopic == kQuitApplication) {
>-      this._bs.removeObserver(this);
>       this._os.removeObserver(this, kQuitApplication);
>       this._prefs.QueryInterface(Ci.nsIPrefBranch2).removeObserver("", this);
>       this._timer.cancel();
>@@ -197,6 +198,24 @@ nsPlacesDBFlush.prototype = {
>   onItemMoved: function() { },
> 
>   //////////////////////////////////////////////////////////////////////////////
>+  //// nsINavHistoryObserver
>+
>+  // We currently only use the history observer to know when the history service
>+  // is activated.  At that point, we actually get initialized, and our timer
>+  // to sync history is added.
>+
>+  // These methods share the name of the ones on nsINavBookmarkObserver, so
>+  // the implementations can be found above.
>+  //onBeginUpdateBatch: function() { },
>+  //onEndUpdateBatch: function() { },
>+  onVisit: function(aURI, aVisitID, aTime, aSessionID, aReferringID, aTransitionType) { },
>+  onTitleChanged: function(aURI, aPageTitle) { },
>+  onDeleteURI: function(aURI) { },
>+  onClearHistory: function() { },
>+  onPageChanged: function(aURI, aWhat, aValue) { },
>+  onPageExpired: function(aURI, aVisitTime, aWholeEntry) { },
>+
>+  //////////////////////////////////////////////////////////////////////////////
>   //// nsITimerCallback
> 
>   notify: function() this._syncTables(["places", "historyvisits"]),
>@@ -315,13 +334,18 @@ nsPlacesDBFlush.prototype = {
>   classDescription: "Used to synchronize the temporary and permanent tables of Places",
>   classID: Components.ID("c1751cfc-e8f1-4ade-b0bb-f74edfb8ef6a"),
>   contractID: "@mozilla.org/places/sync;1",
>-  _xpcom_categories: [{
>-    category: "profile-after-change",
>-  }],
>+
>+  // Registering in these categories makes us get initialized when either of
>+  // those listeners would be notified.
>+  _xpcom_categories: [
>+    { category: "bookmark-observers" },
>+    { category: "history-observers" },
>+  ],
> 
>   QueryInterface: XPCOMUtils.generateQI([
>     Ci.nsIObserver,
>     Ci.nsINavBookmarkObserver,
>+    Ci.nsINavHistoryObserver,
>     Ci.nsITimerCallback,
>     Ci.mozIStorageStatementCallback,
>   ])
>diff --git a/toolkit/components/places/src/nsPlacesMacros.h b/toolkit/components/places/src/nsPlacesMacros.h
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/places/src/nsPlacesMacros.h
>@@ -0,0 +1,82 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Places.
>+ *
>+ * The Initial Developer of the Original Code is Mozilla Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Mark Finkle <mfinkle@mozilla.com> (original author)

you should add yourself as a contributor here.

template specialization + xpcom. so hot and so ugly, at the same time. r=me.
Attachment #380514 - Flags: review?(dietrich) → review+
yeah, definitely intended to cut off the top 50% of that patch before submitting the comment.
Attached patch branch patch v1.1 (obsolete) (deleted) — — Splinter Review 
addresses review comments
Attachment #380514 - Attachment is obsolete: true
Blocks: 476220
yay anyone?

http://hg.mozilla.org/mozilla-central/rev/d891a7418d95
Status: ASSIGNED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Whiteboard: [can land]
Comment on attachment 380839 [details] [diff] [review]
branch patch v1.1

I expect this to sit for a few days in the approval queue, but fennec really really wants this :)
Attachment #380839 - Flags: approval1.9.1?
Attachment #380839 - Flags: approval1.9.1?
Backed this out due to a hang in test_preventive_maintenance.js on windows only twice in a row :(

The best part is that it didn't do this on the try server.  I'm not sure what to do here.
http://hg.mozilla.org/mozilla-central/rev/04040160e7da
http://hg.mozilla.org/mozilla-central/rev/97e7f0da28d6
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
not it did here, i'm going to retest this locally and look into.
it's quite strange, even if test_preventive_maintenance would have hanged, we should at least see some dump from it, at least the dump for the first test.
Instead it is like it has timeout while creating services, since previous test passed and then we see the timeout.
I've run the full Places toolkit unit tests suite multiple times on win without seeing any timeout.
Sorry, i have no idea. All the runs i did succeeded, i can't explain how we could hang between 2 tests, or at least i would expect to hang at random points between different tests at that point. Nor i think test_preventive_maintenance hangs on windows, the test does not even dump anything.
I would suggest to push this again adding more dumps to the above test, to check if we hang while getting services.
We don't get logs automatically dumped like we do for other test harnesses, so adding more logging wouldn't help.
today i see an assertion in test_preventive_maintenance, that could explain the previous hang in the same test.
The problem is bookmarkshash goes out of sync, could be due to a preventive maintenance task, that could explain the previous hang.
Attached patch patch v6.7 (deleted) — — Splinter Review 
unbitrot and fixes for a couple tests. all tests pass locally, so i'm going to retry pushing.
Attachment #380497 - Attachment is obsolete: true
Attachment #380839 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/0f22c55684a6
Status: REOPENED → RESOLVED
Closed: 16 years ago → 15 years ago
Resolution: --- → FIXED
Blocks: 484175
This is wanted1.9.1+ and apparently fixes the problem we're seeing in SeaMonkey as bug 484175, can this be driven into 1.9.1.1?
Attached patch 1.9.1 branch patch (obsolete) (deleted) — — Splinter Review 
This is a combination of Attachment 380839 [details] [diff] and Attachment 387168 [details] [diff]. This is the branch patch plus the fix for test_preventive_maintenance.js so that the test does not timeout (tested this with try server). I did not port the other test fixes and code changes which were not included in the branch patch (for example the one in PlacesSQLQueryBuilder::OrderBy()).
blocking1.9.1: --- → ?
Attachment #388727 - Flags: approval1.9.1.2?
Attachment #388727 - Flags: approval1.9.1.1?
Attachment #388727 - Flags: approval1.9.1.1?
Not blocking and right now we don't intend on taking a ~60kb patch on the 1.9.1 branch. There needs to be quite a bit more baking and a lot more rationale for it. I'd prefer to wait until 1.9.2...
blocking1.9.1: ? → -
status1.9.1: --- → needstriage
Samuel, you're saying that this blocking the next SeaMonkey beta is weak rationale?
Kairo: where on this bug does it say that this bug blocks that particular milestone, or what platform branch you're basing SeaMonkey off of? I don't think Sam was out of line, nor that he was saying what you implied in comment 85.

We're willing to meet you here, but crave information and context.
Mike, comment #82 says this patch solves bug 484175, which is therefore marked as being blocked by this bug, and which by itself is being marked as blocking‑seamonkey2.0b2+ (that flag doesn't exist in toolkit, so can't be set on this bug).
Comment 82 doesn't indicate that it's necessary for the beta, unless you unwind it pretty far and know how the dependency field was being used here.  It's helpful to state your case explicitly in the nomination message, since drivers aren't really able to dig through dependency chains to figure out the motivation!
Sorry, I hope it's clear now that it's a blocker on our side. I thought that being wanted1.9.1+ and having a well-baked patch was enough to get traction, but I guess the size of the patch warrants some additional investigation into what the needs are. The really important flag here is probably the approval request on the patch though, when it's granted I guess the blocking one doesn't matter much ;-)
Attached patch 1.9.1 branch patch (deleted) — — Splinter Review 
Now includes the changes to PlacesSQLQueryBuilder::OrderBy, too.
Attachment #388727 - Attachment is obsolete: true
Attachment #388727 - Flags: approval1.9.1.2?
Comment on attachment 390240 [details] [diff] [review]
1.9.1 branch patch

This bug is blocking the next SeaMonkey 2 beta, see Bug 484175.
Attachment #390240 - Flags: approval1.9.1.2?
Comment on attachment 390240 [details] [diff] [review]
1.9.1 branch patch

We'll look at this for 1.9.1.3 which is targeted for early September. Is that okay for your beta?
Attachment #390240 - Flags: approval1.9.1.2? → approval1.9.1.3?
(In reply to comment #92)
> (From update of attachment 390240 [details] [diff] [review])
> We'll look at this for 1.9.1.3 which is targeted for early September. Is that
> okay for your beta?

Should be OK for final, but not sure about beta2, we don't have a schedule yet, we just know we want to do it ASAP.
Comment on attachment 390240 [details] [diff] [review]
1.9.1 branch patch

Approved for 1.9.1.3. a=ss
Attachment #390240 - Flags: approval1.9.1.3? → approval1.9.1.3+
Checked in Attachment 390240 [details] [diff] on 1.9.1 branch, http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9d4609534f9e
status1.9.1: ? → .3-fixed
I backed this patch out again as the WinXP talos fast tinderbox sometimes crashes during the Ts test with this patch :-|, stack trace is (for full log see http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1250066816.1250072649.23150.gz):
 0  sqlite3.dll!sqlite3_sql [sqlite3.c:194b881050ac : 44888 + 0x4]
 1  xul.dll!mozStorageConnection::ExecuteAsync(mozIStorageStatement * *,unsigned int,mozIStorageStatementCallback *,mozIStoragePendingStatement * *) [mozStorageConnection.cpp:194b881050ac : 399 + 0xd]
 2  xul.dll!NS_InvokeByIndex_P [xptcinvoke.cpp:194b881050ac : 101 + 0x2]
 3  xul.dll!XPCWrappedNative::CallMethod(XPCCallContext &,XPCWrappedNative::CallMode) [xpcwrappednative.cpp:194b881050ac : 2415 + 0x34]
 4  xul.dll!XPCWrappedNative::CallMethod(XPCCallContext &,XPCWrappedNative::CallMode) [xpcwrappednative.cpp:194b881050ac : 2454 + 0x1e]
 5  xul.dll!XPC_WN_CallMethod(JSContext *,JSObject *,unsigned int,int *,int *) [xpcwrappednativejsops.cpp:194b881050ac : 1590 + 0x11]
 6  js3250.dll!js_Invoke [jsinterp.cpp:194b881050ac : 1386 + 0x12]
 7  js3250.dll!js_Interpret [jsinterp.cpp:194b881050ac : 5179 + 0x9]
 8  js3250.dll!js_Invoke [jsinterp.cpp:194b881050ac : 1394 + 0x5]
 9  xul.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS *,unsigned short,XPTMethodDescriptor const *,nsXPTCMiniVariant *) [xpcwrappedjsclass.cpp:194b881050ac : 1697 + 0x11]
10  xul.dll!nsXPCWrappedJS::CallMethod(unsigned short,XPTMethodDescriptor const *,nsXPTCMiniVariant *) [xpcwrappedjs.cpp:194b881050ac : 569 + 0x29]
11  xul.dll!PrepareAndDispatch [xptcstubs.cpp:194b881050ac : 114 + 0x14]
12  xul.dll!SharedStub [xptcstubs.cpp:194b881050ac : 141 + 0x4]
13  xul.dll!nsNavBookmarks::EndUpdateBatch() [nsNavBookmarks.cpp:194b881050ac : 3028 + 0x4d]

Other tinderboxen were fine.
status1.9.1: .3-fixed → ?
Depends on: 463907
Pushed to 1.9.1 branch again http://hg.mozilla.org/releases/mozilla-1.9.1/rev/65b7d8932681, this time with the patch from Bug 463907
status1.9.1: ? → .3-fixed
Verifying this for 1.9.1.3 based on checked in tests passing since there is no manual case.
Keywords: verified1.9.1
Depends on: 516444
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: