831428 - ContentChild's nsIConsoleListener should have threadsafe addref/release methods

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

I'm not familiar with this code at all, but afaict, implementations of nsIConsoleListener should have threadsafe addref/release methods, because the nsIConsoleService is threadsafe and operates on its listener objects in a threadsafe manner.

Patch in a moment.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Patch for b2g18, v1

Not sure who to ask for review here, since the console service is quite old.

Comment 2

[Benjamin Smedberg]

I don't think your understanding is correct. The console service accepts messages from any thread, but console listeners are always called on the main thread (see LogMessageRunnable). I don't think this bug is valid.

Comment 3

[Benjamin Smedberg]

Comment on attachment 702956 [details] [diff] [review]
Patch for b2g18, v1

going to mark r- here, please re-request review if I got something wrong

Comment 4

[Justin Lebar (not reading bugmail)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> I don't think your understanding is correct. The console service accepts
> messages from any thread, but console listeners are always called on the
> main thread (see LogMessageRunnable). I don't think this bug is valid.

The listeners are always /called/ on the main thread, but they are /addref'ed/ off the main thread, which is what causes the problem.  See the stack in bug 819000 comment 32.

Comment 5

[Benjamin Smedberg]

Attachment: Threadsafe addref/release not required, rev. 1

I'll counter-propose this so that listeners don't have to have threadsafe addref/release.

Comment 6

[Justin Lebar (not reading bugmail)]

Comment on attachment 702956 [details] [diff] [review]
Patch for b2g18, v1

Even better.

>@@ -209,29 +209,36 @@ nsConsoleService::LogMessageWithMode(nsI
>          * Copy the listeners into the snapshot array - in case a listener
>          * is removed during an Observe(...) notification. If there are no
>          * listeners, don't bother to create the Runnable, since we don't
>          * need to run it and it will hold onto the memory for the message
>          * unnecessarily.
>          */
>         if (mListeners.Count() > 0) {
>             r = new LogMessageRunnable(message, this);
>-            mListeners.EnumerateRead(CollectCurrentListeners, r);
>         }

Update this comment?

Comment 7

[Justin Lebar (not reading bugmail)]

Comment on attachment 702956 [details] [diff] [review]
Patch for b2g18, v1

Whoops, wrong patch.

Comment 8

[Benjamin Smedberg]

Attachment: Test fixup in content/base, rev. 1

bz, you wrote this test originally, can you quickly look at this?

Comment 9

[Benjamin Smedberg]

Attachment: Test fixup for a webconsole test, rev. 1

Rob, this changes console delivery order in the following case:

* message is reported
* console listener is added

In the past, the new console listener would not have received the message, while now it does (we notify all listeners active at the time of *delivery* not the time the message is reported). I believe that the new behavior is slightly more correct (and fixes this bug), so I needed to refactor this test slightly because you were using mutation events (which produce a console warning) and then adding the console listener. The test now uses a mutation listener instead.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 703922 [details] [diff] [review]
Test fixup in content/base, rev. 1

Huh.  I have no idea what I was thinking when I wrote that...

r=me

Comment 11

[Boris Zbarsky [:bzbarsky]]

Oh, I see why it worked before.  Yeah, ok.

Comment 12

[Justin Lebar (not reading bugmail)]

I don't have a strong opinion on this, but what would you think of landing the threadsafe addref/release patch on b2g18 and the "right fix" on trunk?  I'm afraid that if we have to take this bigger change we might not take the fix at all, which would be a bummer wrt to bug 819000.  OTOH there's risk when we diverge trunk and b2g18.

Comment 13

[Benjamin Smedberg]

oh, I didn't realize this was related to urgent B2G issues. Yeah, the threadsafe refcounting of ChildChild should be fine as a quick workaround. ContentChild is not a cycle-collected class, right?

Comment 14

[Justin Lebar (not reading bugmail)]

Comment on attachment 702956 [details] [diff] [review]
Patch for b2g18, v1

[Approval Request Comment]
This simple patch is needed for bug 819000.  We have a more correct patch we're working on for trunk.

Comment 15

[Benjamin Smedberg]

Attachment: Fix browser_Deprecated.js, rev. 1

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 702956 [details] [diff] [review]
Patch for b2g18, v1

Approving for landing to b2g18 branch since it's required for bug 819000

Comment 17

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/57fc020315a0

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 704919 [details] [diff] [review]
Fix browser_Deprecated.js, rev. 1

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

I'll need some context here. What was broken in the test?

Comment 19

[Benjamin Smedberg]

Comment on attachment 704919 [details] [diff] [review]
Fix browser_Deprecated.js, rev. 1

Please see comment 9. The timing of when we collect the console listeners has changed slightly. You can't create an error and then immediately remove the console listener.

Comment 20

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 703926 [details] [diff] [review]
Test fixup for a webconsole test, rev. 1

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

::: browser/devtools/webconsole/test/browser_webconsole_bug_595934_message_categories.js
@@ +131,5 @@
>  };
>  
>  function consoleOpened(hud) {
>    output = hud.outputNode;
> +  

got a little whitespace there.

Comment 21

[Rob Campbell [:rc] (:robcee)]

in the future, you should direct a review towards Mihai Sucan when making any console-related changes. Thanks!

Comment 22

[Justin Lebar (not reading bugmail)]

David, we have a fair bit of code in bug 819000 that we can't land on m-c because we're waiting on this review.  That's causing some scary B2G bits to be tested less than we'd like.  If you can't get to this review soon, is there anyone else who could do it?

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 704919 [details] [diff] [review]
Fix browser_Deprecated.js, rev. 1

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

(sorry, I must have forgotten to click "publish")
Looks good to me.

Comment 24

[Justin Lebar (not reading bugmail)]

bsmedberg, are you ready to land this?  It would be really nice if we could land bug 819000 on m-c.

Comment 25

[Benjamin Smedberg]

https://hg.mozilla.org/integration/mozilla-inbound/rev/507d85ab5807
https://hg.mozilla.org/integration/mozilla-inbound/rev/783ea1637777
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c5b63581fc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8b0ebdfe845

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/507d85ab5807
https://hg.mozilla.org/mozilla-central/rev/5c5b63581fc2
https://hg.mozilla.org/mozilla-central/rev/f8b0ebdfe845

Comment 27

[Jason Barnabe (np)]

In addition to the behavioural change described in comment 9, this appears to also change the following case:

* message is reported
* console listener is removed immediately after

Whereas before the listener would receive the message, now it no longer does. Is the only solution to do as suggested in bug 852220 comment 0 and http://forums.mozillazine.org/viewtopic.php?p=12740837#p12740837 - remove the console listener on a timeout?

Comment 28

[Benjamin Smedberg]

np, that is the desired change. We really don't envision short-lived console listeners, and so if you have specific messages you want to view, you will need to make sure your console listener lasts at least until the message is dispatched.