Since bug 461199 landed there have been constant failures in Thunderbird builds. The bloat builds are failing with: WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040154: file /Users/moztest/comm/main/src/mozilla/content/base/src/nsContentUtils.cpp, line 381 ###!!! ASSERTION: Could not initialize nsContentUtils: 'Error', file /Users/moztest/comm/main/src/mozilla/layout/build/nsLayoutStatics.cpp, line 168 (it then crashes, but that's a separate bug). I've tracked the issue down to: http://hg.mozilla.org/mozilla-central/annotate/35059e8e8ce8/content/base/src/nsContentUtils.cpp#l380 That is trying to get the new IHistory service, however that service is only implemented in places, and Thunderbird doesn't build places. As MOZ_PLACES still seems to be supported, I think the solution would be to ifdef the relevant locations, or possibly to provide a stub service (I don't think Thunderbird is in the right position to consider taking up building of places at the moment).

The ifdefs would be something like this (I've not built/tested this yet).

Comment on attachment 427577 [details] [diff] [review] Possible fix This fixes the Thunderbird builds so they run. I haven't run tests yet on the Thunderbird builds (recompiling to do so).

I've just noticed that for some reason in the patched version the links aren't styled. Any ideas?

Without IHistory, you won't get link coloring at all.

Shouldn't you still get all links colored unvisited?

(In reply to comment #5) > Shouldn't you still get all links colored unvisited? Er, yeah; that's what I was trying to get across. Sorry for the confusion!

Thunderbird doesn't have an nsIGlobalHistory2 instance, however its possible that someone building without places wants to have history. So if we don't have places, default to unvisited, and ask the nsIGlobalHistory2 service for the visited status. This will fix the current bustage on Thunderbird trunk. I don't think I can check it in as a bustage fix as its not Firefox bustage ;-) (I'd still like to get some eyes over it anyway).

(In reply to comment #7) > Created an attachment (id=429356) [details] > This will fix the current bustage on Thunderbird trunk... Thanks, your patch does fix the thunderbird problem from my Bug 549101. I notice that thunderbird's 'configure' recognizes the --enable-places flag but I'm having a hard time figuring out how a mail-news reader would use it. Thanks.

(In reply to comment #9) > (In reply to comment #7) > > Created an attachment (id=429356) [details] [details] > > This will fix the current bustage on Thunderbird trunk... > > Thanks, your patch does fix the thunderbird problem from my Bug 549101. > > I notice that thunderbird's 'configure' recognizes the --enable-places flag but > I'm having a hard time figuring out how a mail-news reader would use it. > > Thanks. The mail start page, for one instance. It would be nice to also see a history in the evaluate box of the error console, although that is probably beyond the scope of "history" These are just 2 places that I have to constantly refer to externally saved data.

(In reply to comment #9) > (In reply to comment #7) > > Created an attachment (id=429356) [details] [details] > > This will fix the current bustage on Thunderbird trunk... > > Thanks, your patch does fix the thunderbird problem from my Bug 549101. Are you sure you applied it to the mozilla/ sub directory of the build? and did a full dep build? > I notice that thunderbird's 'configure' recognizes the --enable-places flag but > I'm having a hard time figuring out how a mail-news reader would use it. There are various places in the add-on space that could potentially use it (thunderbrowse would be one-example), but that's not on discussion here.

(In reply to comment #11) > (In reply to comment #9) > > (In reply to comment #7) > > > Created an attachment (id=429356) [details] [details] [details] > > > This will fix the current bustage on Thunderbird trunk... > > > > Thanks, your patch does fix the thunderbird problem from my Bug 549101. > > Are you sure you applied it to the mozilla/ sub directory of the build? and > did a full dep build? I'm a bit confused by your question, which seems to imply that your patch failed, but it didn't fail, it worked perfectly. Which of us is the more confused? ;)

That patch will make it so in non-places builds links no longer go dynamically from unvisited to visited when they're visited. While that's better than not compiling (so I may be ok with this patch temporarily just to fix the bustage), it doesn't quite seem like a mode we want to support in Gecko, especially since we actually have to have more code to support it. Is there a reason Tbird can't implement IHistory, exactly? The intent was that IHistory is the history interface that Gecko requires for visited checking, period.

(In reply to comment #13) > While that's better than not compiling (so I may be ok with this patch > temporarily just to fix the bustage), it doesn't quite seem like a mode we want > to support in Gecko, especially since we actually have to have more code to > support it. Is there a reason Tbird can't implement IHistory, exactly? The > intent was that IHistory is the history interface that Gecko requires for > visited checking, period. Well, we can implement IHistory in Thunderbird (in the future we'd probably move to places, but that's a different bug/timescale as previously mentioned above). However, what about the gecko embedding story/integration with other apps? For anything not xulrunner/Firefox places is disabled by default. Maybe that's history as to why its like that, but still if disabling places is supported, then shouldn't there be something that implements IHistory?

This implements IHistory in Thunderbird only (sorry embedding apps) with an empty implementation so that content is happy and we can start up and run. Links by default then get left as unvisited. There's no expectation for reference counting, so we're fine that we've just got empty functions.

If we're still supporting the --disable-places case in 1.9.3, then we should probably add an IHistory shim that builds in that case, yeah. Shawn?

(In reply to comment #16) > If we're still supporting the --disable-places case in 1.9.3, then we should > probably add an IHistory shim that builds in that case, yeah. Shawn? Yup; I'll wait to hear from bsmedberg.

I actively oppose falling back to nsIGlobalHistory*. I don't really *want* to support --disable-places: our platform suffers from too many feature ifdefs, and that seems like an obvious one to kill.

Comment on attachment 429509 [details] [diff] [review] Implement in Thunderbird +REQUIRES = \ + xpcom \ + string \ + layers \ + $(NULL) I'd be shocked and appalled if we really REQUIRE layers, and I don't think we REQUIRE strings either r/sr=me with that nit. It definitely fixes the crash on startup.

REQUIRES doesn't exist on trunk or 1.9.2... is this supposed to be backwards-compatible with the 1.9.1 codebase?

(In reply to comment #18) > I actively oppose falling back to nsIGlobalHistory*. I don't really *want* to > support --disable-places: our platform suffers from too many feature ifdefs, > and that seems like an obvious one to kill. We can consider enabling it, but I think now isn't the right time for us. Whilst it would be easy to do, there are various aspects we should consider before enabling it in Thunderbird. I guess for now we could kill the configure option but support the define, or just switch the option so that the default is to enable places... (which it isn't for non-FF/xulrunner apps). Anyway, that's a different bug. (In reply to comment #19) > (From update of attachment 429509 [details] [diff] [review]) > +REQUIRES = \ ... > I'd be shocked and appalled if we really REQUIRE layers, and I don't think we > REQUIRE strings either Yeah, that was a mistake, I've just removed the whole REQUIRES.

I checked this in the other day: http://hg.mozilla.org/comm-central/rev/5fad69db4a71