687696 - test_IHistory.cpp uses places without a profile

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

see https://bugzilla.mozilla.org/show_bug.cgi?id=673017#c307 as to why it should have one.

Comment 1

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

It looks like test_IHistory *has* a profile. The issues are

* the shutdown messages not being sent
* The bookmark, annotation and favicon services not being used by the test (they are lazily initiated).

Comment 2

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: proposed patch

Comment 3

[Marco Bonardo [:mak]]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #1)
> * The bookmark, annotation and favicon services not being used by the test
> (they are lazily initiated).

Why should this matter?

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Try at
https://tbpl.mozilla.org/?tree=Try&rev=4447502e058f

Comment 5

[Marco Bonardo [:mak]]

Ah ok, I see, you just don't want to startup the services if they are not needed. that's ok.
The shotdown observer should be moved to places_test_harness.h and places_test_harness_tail.h so that any newly added test will use it, I don't understand why it has to listen to both will-shutdown and shutdown, could't it just listen to xpcom-will-shutdown and notify both profile notifications with NotifyObservers? The order should be guarantee

Comment 6

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Marco Bonardo [:mak] from comment #3)
> (In reply to Rafael Ávila de Espíndola (:espindola) from comment #1)
> > * The bookmark, annotation and favicon services not being used by the test
> > (they are lazily initiated).
> 
> Why should this matter?

The getter tries to start those during shutdown...

Comment 7

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: new patch

Comment 8

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

try at https://tbpl.mozilla.org/?tree=Try&rev=2a058858b9d4

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 561195 [details] [diff] [review]
new patch

Review of attachment 561195 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsNavHistory.cpp
@@ +5129,4 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    // nsNavBookmarks
> +  nsNavBookmarks *bookmarks = nsNavBookmarks::GetBookmarksServiceIfAvailable();

while here, please fix pointer style (correct is toward type)
and remove these useless comments like // nsNavBookmarks, // nsAnnotationService, // nsFaviconService... they are just anticipating what is written on the immediately following line.

::: toolkit/components/places/tests/cpp/places_test_harness.h
@@ +338,5 @@
> +class ShutdownObserver : public nsIObserver
> +{
> +public:
> +  NS_DECL_ISUPPORTS
> +  ShutdownObserver() {

newline between decl and between methods (also the other method below) pls
brace on newline too

@@ +340,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +  ShutdownObserver() {
> +    nsCOMPtr<nsIObserverService> observerService =
> +      do_GetService(NS_OBSERVERSERVICE_CONTRACTID);

nit: don't remember if in this specific harness we may use mozilla::services, it would be nice.

@@ +350,5 @@
> +  NS_IMETHOD Observe(nsISupports* aSubject,
> +                     const char* aTopic,
> +                     const PRUnichar* aData)
> +  {
> +    if (strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID) == 0) {

I think it would be a bit more real if we listen to will-shutdown rather than shutdown (since ideally profile notifications are supposed to happen before)

@@ +354,5 @@
> +    if (strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID) == 0) {
> +      nsCOMPtr<nsIObserverService> os =
> +        do_GetService(NS_OBSERVERSERVICE_CONTRACTID);
> +      os->NotifyObservers(nsnull, TOPIC_PROFILE_TEARDOWN, nsnull);
> +      os->NotifyObservers(nsnull, TOPIC_PROFILE_CHANGE, nsnull);

please cast to (void) if you don't care about result value, per our code style

Comment 10

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/cec49ea0739d

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cec49ea0739d

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 561195 [details] [diff] [review]
new patch

>--- a/toolkit/components/places/tests/cpp/places_test_harness_tail.h
>+++ b/toolkit/components/places/tests/cpp/places_test_harness_tail.h
>@@ -115,6 +115,8 @@ main(int aArgc,
>      char** aArgv)
> {
>   ScopedXPCOM xpcom(TEST_NAME);
>+  nsCOMPtr<ShutdownObserver> shutdownObserver = new ShutdownObserver();

ShutdownObserver is not an interface, so nsCOMPtr<ShutdownObserver> is illegal.

Comment 13

[Marco Bonardo [:mak]]

(In reply to Ms2ger from comment #12)
> ShutdownObserver is not an interface, so nsCOMPtr<ShutdownObserver> is
> illegal.

Right, actually it will work correctly but should be a nsRefPtr, thanks for pointing that out.

Comment 14

[Ed Morley [:emorley]]

Not sure if this or bug 683400 is responsible for the recent Ts regression:

https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/WOlX-ghe0H4

http://graphs-new.mozilla.org/graph.html#tests=[[54,63,14],[54,63,15]]&sel=1316423909867.185,1316699577328&displayrange=7&datatype=running

Comment 15

[Marco Bonardo [:mak]]

(In reply to Ed Morley [:edmorley] from comment #14)
> Not sure if this or bug 683400 is responsible for the recent Ts regression:
> 
> https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/WOlX-
> ghe0H4
> 
> http://graphs-new.mozilla.org/graph.html#tests=[[54,63,14],[54,63,
> 15]]&sel=1316423909867.185,1316699577328&displayrange=7&datatype=running

Or just the Talos downtime yesterday. this bug can
t be responsible for any Talos change, it's a test-only fix.

Comment 16

[Marco Bonardo [:mak]]

ehr, nevermind, it modifies the shutdown process. But ideally can't make it slower since in the worst case is same as before.

Comment 17

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Use nsRefPtr

Comment 18

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/96e0836ed625