Last Comment Bug 831428 - ContentChild's nsIConsoleListener should have threadsafe addref/release methods
: ContentChild's nsIConsoleListener should have threadsafe addref/release methods
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla21
Assigned To: Benjamin Smedberg [:bsmedberg]
:
Mentors:
Depends on: 852220
Blocks: 819000 835809
  Show dependency treegraph
 
Reported: 2013-01-16 11:55 PST by Justin Lebar (not reading bugmail)
Modified: 2013-03-21 08:53 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
fixed
fixed
fixed


Attachments
Patch for b2g18, v1 (1.81 KB, patch)
2013-01-16 11:58 PST, Justin Lebar (not reading bugmail)
benjamin: review+
lukasblakk+bugs: approval‑mozilla‑b2g18+
Details | Diff | Review
Threadsafe addref/release not required, rev. 1 (4.56 KB, patch)
2013-01-16 15:00 PST, Benjamin Smedberg [:bsmedberg]
justin.lebar+bug: review+
Details | Diff | Review
Test fixup in content/base, rev. 1 (1.69 KB, patch)
2013-01-18 08:41 PST, Benjamin Smedberg [:bsmedberg]
bzbarsky: review+
Details | Diff | Review
Test fixup for a webconsole test, rev. 1 (2.94 KB, patch)
2013-01-18 08:44 PST, Benjamin Smedberg [:bsmedberg]
rcampbell: review+
Details | Diff | Review
Fix browser_Deprecated.js, rev. 1 (3.60 KB, patch)
2013-01-22 09:04 PST, Benjamin Smedberg [:bsmedberg]
dteller: review+
dteller: review+
Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2013-01-16 11:55:35 PST
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) 2013-01-16 11:58:38 PST
Created attachment 702956 [details] [diff] [review]
Patch for b2g18, v1

Not sure who to ask for review here, since the console service is quite old.
Comment 2 Benjamin Smedberg [:bsmedberg] 2013-01-16 12:34:14 PST
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 [:bsmedberg] 2013-01-16 12:37:27 PST
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) 2013-01-16 12:40:13 PST
(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 [:bsmedberg] 2013-01-16 15:00:07 PST
Created attachment 703051 [details] [diff] [review]
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) 2013-01-17 06:55:22 PST
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) 2013-01-17 06:55:43 PST
Comment on attachment 702956 [details] [diff] [review]
Patch for b2g18, v1

Whoops, wrong patch.
Comment 8 Benjamin Smedberg [:bsmedberg] 2013-01-18 08:41:51 PST
Created attachment 703922 [details] [diff] [review]
Test fixup in content/base, rev. 1

bz, you wrote this test originally, can you quickly look at this?
Comment 9 Benjamin Smedberg [:bsmedberg] 2013-01-18 08:44:39 PST
Created attachment 703926 [details] [diff] [review]
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 [:bz] 2013-01-18 08:45:51 PST
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 [:bz] 2013-01-18 08:46:21 PST
Oh, I see why it worked before.  Yeah, ok.
Comment 12 Justin Lebar (not reading bugmail) 2013-01-18 10:10:19 PST
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 [:bsmedberg] 2013-01-18 10:17:41 PST
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) 2013-01-18 14:02:55 PST
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 [:bsmedberg] 2013-01-22 09:04:48 PST
Created attachment 704919 [details] [diff] [review]
Fix browser_Deprecated.js, rev. 1
Comment 16 Lukas Blakk [:lsblakk] use ?needinfo 2013-01-22 12:55:07 PST
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) 2013-01-22 13:21:20 PST
https://hg.mozilla.org/releases/mozilla-b2g18/rev/57fc020315a0
Comment 18 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-01-23 05:58:23 PST
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 [:bsmedberg] 2013-01-23 06:13:32 PST
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) 2013-01-23 13:28:45 PST
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) 2013-01-23 13:30:43 PST
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) 2013-01-25 07:53:43 PST
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 Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-01-25 11:43:05 PST
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) 2013-01-29 06:38:53 PST
bsmedberg, are you ready to land this?  It would be really nice if we could land bug 819000 on m-c.
Comment 27 Jason Barnabe (np) 2013-03-21 08:42:51 PDT
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 [:bsmedberg] 2013-03-21 08:53:38 PDT
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.

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