546913 - Trunk builds without Places failing due to not being able to initialise the IHistory service

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Mark Banner (:standard8)]

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

Comment 1

[Mark Banner (:standard8)]

Attachment: Possible fix

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

Comment 2

[Mark Banner (:standard8)]

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

Comment 3

[Mark Banner (:standard8)]

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

Comment 4

[Shawn Wilsher :sdwilsh]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

Shouldn't you still get all links colored unvisited?

Comment 6

[Shawn Wilsher :sdwilsh]

(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!

Comment 7

[Mark Banner (:standard8)]

Attachment: Proposed fix

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

Comment 9

[walter sheets]

(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.

Comment 10

[Joe Sabash [:JoeS1]]

(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.

Comment 11

[Mark Banner (:standard8)]

(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.

Comment 12

[walter sheets]

(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? ;)

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[Mark Banner (:standard8)]

(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?

Comment 15

[Mark Banner (:standard8)]

Attachment: Implement in Thunderbird

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

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?

Comment 17

[Shawn Wilsher :sdwilsh]

(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.

Comment 18

[Benjamin Smedberg]

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 19

[David :Bienvenu]

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.

Comment 20

[Benjamin Smedberg]

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

Comment 21

[Mark Banner (:standard8)]

(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.

Comment 22

[Mark Banner (:standard8)]

I checked this in the other day:

http://hg.mozilla.org/comm-central/rev/5fad69db4a71