712767 - Send profile-before-change in netwerk/test/TestSTSParser.cpp

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

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

This makes sure that the sql databases are shutdown.

Comment 1

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

Attachment: Send profile-before-change in netwerk/test/TestSTSParser.cpp

https://tbpl.mozilla.org/?tree=Try&rev=47252c439713

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 583617 [details] [diff] [review]
Send profile-before-change in netwerk/test/TestSTSParser.cpp

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

::: netwerk/test/TestSTSParser.cpp
@@ +196,5 @@
>        passed("Avoided parsing invalid STS headers");
>  
> +    nsCOMPtr<nsIObserverService> os =
> +      do_GetService(NS_OBSERVERSERVICE_CONTRACTID);
> +    (void)os->NotifyObservers(nsnull, TOPIC_PROFILE_CHANGE, nsnull);

I'd prefer if you'd do the same I did in test_IHistory, where I created a ProfileScopedXPCOM class, see http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/cpp/places_test_harness.h#336

The problem is that there is no need to duplicate all that code, we should just move that class to xpcom/tests/TestHarness.h and then use it here.
This patch would then reduce to a single line change.

Comment 3

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

Attachment: Move ProfileScopedXPCOM. Use it in TestSTSParser.cpp.

https://tbpl.mozilla.org/?tree=Try&rev=5ef4720a5d25

Comment 4

[Marco Bonardo [:mak]]

hm, TestHarness should not depend on Places stuff... so, I guess we need another set of inheritance, ProfileScopedXPCOM should only fire the notifications but not have all the code to close the places connection (that should stay in places_test_harness.h)

So either Places test harness further inherits from ProfileScopedXPCOM, or we do the spinning elsewhere (like in a profile-before-change observer). Ideas?

Comment 5

[Marco Bonardo [:mak]]

so, probably in places_test_harness.h we should add a "places-will-close-connection" observer, that spins till "places-connection-closed" is received

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 584679 [details] [diff] [review]
Move ProfileScopedXPCOM. Use it in TestSTSParser.cpp.

this was a minus, fwiw.

Comment 7

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

Attachment: Send profile-before-change in netwerk/test/TestSTSParser.cpp

I will push to try once it is back.

Comment 8

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

Attachment: Move ProfileScopedXPCOM. Use it in TestSTSParser.cpp

https://tbpl.mozilla.org/?tree=Try&rev=c3b78c32bfa7

Comment 9

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

Attachment: Move ProfileScopedXPCOM. Use it in TestSTSParser.cpp

https://tbpl.mozilla.org/?tree=Try&rev=c98f9f5a7a5e

Comment 10

[Marco Bonardo [:mak]]

please, stop a while looking at this, I'm trying an alternative solution and I don't want to waste your time on ideas :)

Comment 11

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

(In reply to Marco Bonardo [:mak] from comment #10)
> please, stop a while looking at this, I'm trying an alternative solution and
> I don't want to waste your time on ideas :)

Would you mind reviewing this one first? If your idea works and is better we can always improve the code with it. I really don't consider my time being wasted if I unblock other work earlier, even if the code is completely deleted the next day.

Comment 12

[Marco Bonardo [:mak]]

I reviewed this first obviously, the problem is that we are going far from the common test harness, creating unwanted differences between them, and I'd prefer to keep things similar instead.
TestHarness.h is mostly a cpp porting of head.js (with obvious differenced) so looking again at it, I think they should keep sharing a common behavior. When head.js gets a profile request, it fires the profile topics on shutdown, ScopedXPCOM should just do the same, notice that in ~ScopedXPCOM() it already partially does that. This is easy, I have it already done, but Places cpp test harness needs to hook into that, that is the tricky part, thus I'm just investigating with an observing and spinning object.
Btw, don't worry about time, I may have an answer shortly.

Finally, even if I review this we would need a second review from waldo that wrote the harness, or any other xpcom peer, so it couldn't land yet.

Comment 13

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

How about landing the original fix that just sends the message in one test while you try to figure this out?

This code review behavior is *really* harmful to productivity :-(

Comment 14

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

The patch I propose landing now is the one in

https://tbpl.mozilla.org/?tree=Try&rev=47252c439713

which wouldn't require waldo's review, doesn't interfere with your refactoring, has run on try and is ready since 21th!

Comment 15

[Marco Bonardo [:mak]]

Attachment: test patch

thoughts about this? Could you test it?

actually it works because we are lucky and the harness observer is invoked after the Places ones... otherwise we'd need to reintroduce the places-connection-closing topic after asyncClose() is invoked, that we just removed a month ago...

Comment 16

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

Comment on attachment 585039 [details] [diff] [review]
test patch

My only though about it is that it works, all connections used by TestSTSParser are closed.

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 585039 [details] [diff] [review]
test patch

try says green too

going through waldo that wrote the test harness, not sure what's his right bugmail though...

Comment 18

[Jeff Walden [:Waldo]]

Yeah, that's the right bugmail.  :-)  I'll try to get to this early next week.

Comment 19

[Jeff Walden [:Waldo]]

Comment on attachment 585039 [details] [diff] [review]
test patch

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

::: netwerk/test/TestSTSParser.cpp
@@ +115,5 @@
>      ScopedXPCOM xpcom("STS Parser Tests");
>      if (xpcom.failed())
>        return -1;
> +    // Initialize a profile folder to ensure a clean shutdown.
> +    (void)xpcom.GetProfileDirectory();

If GetProfileDirectory can fail, which it can, you should return -1 or something if it does.  Don't cast away errors like this when they can and should be reported.

::: toolkit/components/places/tests/cpp/places_test_harness.h
@@ +341,4 @@
>    {
> +    nsCOMPtr<nsIObserverService> os =
> +      do_GetService(NS_OBSERVERSERVICE_CONTRACTID);
> +    (void)os->AddObserver(this, TOPIC_PROFILE_CHANGE, false);

If you're assuming this will succeed, assert that it will succeed.  Don't just cast away errors when you can check for them.

@@ +349,5 @@
> +                     const PRUnichar* aData)
> +  {
> +    nsCOMPtr<nsIObserverService> os =
> +      do_GetService(NS_OBSERVERSERVICE_CONTRACTID);
> +    (void)os->RemoveObserver(this, aTopic);

Again, assert that this succeeded, don't just assume that it did.

::: toolkit/components/places/tests/cpp/places_test_harness_tail.h
@@ +117,5 @@
> +  ScopedXPCOM xpcom(TEST_NAME);
> +  if (xpcom.failed())
> +    return -1;
> +    // Initialize a profile folder to ensure a clean shutdown.
> +  (void)xpcom.GetProfileDirectory();

Don't cast away this error either.

Comment 20

[Marco Bonardo [:mak]]

I'm stealing the bug, or I risk to lose it from my radar. I'll address comments now.

Comment 21

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Comment 22

[Jeff Walden [:Waldo]]

Comment on attachment 588072 [details] [diff] [review]
patch v1.1

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

::: netwerk/test/TestSTSParser.cpp
@@ +117,5 @@
>        return -1;
> +    // Initialize a profile folder to ensure a clean shutdown.
> +    nsCOMPtr<nsIFile> profile = xpcom.GetProfileDirectory();
> +    if (!profile)
> +      return -1;

Come to think of it, you should also insert a |fail("couldn't get the profile directory")| so that more gets communicated than just the failed return code.

But: it'd be better if GetProfileDirectory did its own error message printing.  It knows better than this code does what went wrong, so it should be doing the error printing.  (Test harness APIs really should always do this.)  File a followup that does that and removes the fail() that would become redundant here?  I can review that too.

Comment 23

[Marco Bonardo [:mak]]

Attachment: patch v1.2

addressed comment, will file the bug to move up the error message to the harness.

Comment 24

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38b4039f740e

and filed bug 719768 to upstream the failure message to the harness.

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/38b4039f740e

Comment 26

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Ed Morley [:edmorley] from comment #25)
> https://hg.mozilla.org/mozilla-central/rev/38b4039f740e

I think this changeset may have caused build problems when tests are enabled: bug 726372.