Last Comment Bug 712767 - Send profile-before-change in netwerk/test/TestSTSParser.cpp
: Send profile-before-change in netwerk/test/TestSTSParser.cpp
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Marco Bonardo [::mak]
:
Mentors:
Depends on:
Blocks: 719768
  Show dependency treegraph
 
Reported: 2011-12-21 13:57 PST by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2012-02-11 17:58 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Send profile-before-change in netwerk/test/TestSTSParser.cpp (1.81 KB, patch)
2011-12-21 14:00 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
mak77: review-
Details | Diff | Splinter Review
Move ProfileScopedXPCOM. Use it in TestSTSParser.cpp. (7.08 KB, patch)
2011-12-28 18:06 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
mak77: review-
Details | Diff | Splinter Review
Send profile-before-change in netwerk/test/TestSTSParser.cpp (3.88 KB, patch)
2011-12-30 06:20 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
Move ProfileScopedXPCOM. Use it in TestSTSParser.cpp (3.88 KB, patch)
2011-12-30 10:11 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
Move ProfileScopedXPCOM. Use it in TestSTSParser.cpp (3.86 KB, patch)
2011-12-30 12:01 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
test patch (5.03 KB, patch)
2011-12-30 13:46 PST, Marco Bonardo [::mak]
jwalden+bmo: review-
respindola: feedback+
Details | Diff | Splinter Review
patch v1.1 (5.42 KB, patch)
2012-01-12 09:29 PST, Marco Bonardo [::mak]
jwalden+bmo: review+
Details | Diff | Splinter Review
patch v1.2 (4.60 KB, patch)
2012-01-20 05:15 PST, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-21 13:57:50 PST
This makes sure that the sql databases are shutdown.
Comment 1 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-21 14:00:39 PST
Created attachment 583617 [details] [diff] [review]
Send profile-before-change in netwerk/test/TestSTSParser.cpp

https://tbpl.mozilla.org/?tree=Try&rev=47252c439713
Comment 2 Marco Bonardo [::mak] 2011-12-28 05:33:20 PST
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) 2011-12-28 18:06:48 PST
Created attachment 584679 [details] [diff] [review]
Move ProfileScopedXPCOM. Use it in TestSTSParser.cpp.

https://tbpl.mozilla.org/?tree=Try&rev=5ef4720a5d25
Comment 4 Marco Bonardo [::mak] 2011-12-29 06:41:51 PST
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] 2011-12-29 06:46:48 PST
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] 2011-12-30 05:12:59 PST
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) 2011-12-30 06:20:09 PST
Created attachment 584956 [details] [diff] [review]
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) 2011-12-30 10:11:54 PST
Created attachment 584983 [details] [diff] [review]
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) 2011-12-30 12:01:48 PST
Created attachment 585013 [details] [diff] [review]
Move ProfileScopedXPCOM. Use it in TestSTSParser.cpp

https://tbpl.mozilla.org/?tree=Try&rev=c98f9f5a7a5e
Comment 10 Marco Bonardo [::mak] 2011-12-30 12:36:31 PST
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) 2011-12-30 12:43:01 PST
(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] 2011-12-30 12:56:56 PST
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) 2011-12-30 13:01:54 PST
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) 2011-12-30 13:04:02 PST
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] 2011-12-30 13:46:36 PST
Created attachment 585039 [details] [diff] [review]
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) 2011-12-30 15:53:24 PST
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] 2011-12-30 16:54:04 PST
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] (remove +bmo to email) 2012-01-06 22:07:47 PST
Yeah, that's the right bugmail.  :-)  I'll try to get to this early next week.
Comment 19 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-09 13:33:16 PST
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] 2012-01-12 08:02:41 PST
I'm stealing the bug, or I risk to lose it from my radar. I'll address comments now.
Comment 21 Marco Bonardo [::mak] 2012-01-12 09:29:14 PST
Created attachment 588072 [details] [diff] [review]
patch v1.1
Comment 22 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-18 14:25:01 PST
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] 2012-01-20 05:15:43 PST
Created attachment 590173 [details] [diff] [review]
patch v1.2

addressed comment, will file the bug to move up the error message to the harness.
Comment 24 Marco Bonardo [::mak] 2012-01-20 05:49:26 PST
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] 2012-01-21 06:50:06 PST
https://hg.mozilla.org/mozilla-central/rev/38b4039f740e
Comment 26 Tim Abraldes [:TimAbraldes] [:tabraldes] 2012-02-11 17:58:24 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.