Closed Bug 831428 Opened 7 years ago Closed 7 years ago

ContentChild's nsIConsoleListener should have threadsafe addref/release methods

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- fixed

People

(Reporter: justin.lebar+bug, Assigned: benjamin)

References

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(5 files)

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.
Not sure who to ask for review here, since the console service is quite old.
Attachment #702956 - Flags: review?(benjamin)
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 on attachment 702956 [details] [diff] [review]
Patch for b2g18, v1

going to mark r- here, please re-request review if I got something wrong
Attachment #702956 - Flags: review?(benjamin) → review-
(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.
Attachment #702956 - Flags: review- → review?(benjamin)
Summary: ContentChild's nsIConsoleListener should be threadsafe → ContentChild's nsIConsoleListener should have threadsafe addref/release methods
I'll counter-propose this so that listeners don't have to have threadsafe addref/release.
Attachment #703051 - Flags: review?(justin.lebar+bug)
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?
Attachment #702956 - Flags: review?(benjamin) → review+
Comment on attachment 702956 [details] [diff] [review]
Patch for b2g18, v1

Whoops, wrong patch.
Attachment #702956 - Attachment is obsolete: true
Attachment #702956 - Flags: review+
Attachment #703051 - Flags: review?(justin.lebar+bug) → review+
bz, you wrote this test originally, can you quickly look at this?
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
Attachment #703922 - Flags: review?(bzbarsky)
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.
Attachment #703926 - Flags: review?(rcampbell)
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
Attachment #703922 - Flags: review?(bzbarsky) → review+
Oh, I see why it worked before.  Yeah, ok.
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.
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 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.
Attachment #702956 - Attachment description: Patch, v1 → Patch for b2g18, v1
Attachment #702956 - Flags: review?(benjamin)
Attachment #702956 - Flags: approval-mozilla-b2g18?
Attachment #702956 - Attachment is obsolete: false
Attachment #702956 - Flags: review?(benjamin) → review+
Attachment #704919 - Flags: review?(dteller)
Comment on attachment 702956 [details] [diff] [review]
Patch for b2g18, v1

Approving for landing to b2g18 branch since it's required for bug 819000
Attachment #702956 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
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?
Attachment #704919 - Flags: review?(dteller)
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.
Attachment #704919 - Flags: review?(dteller)
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.
Attachment #703926 - Flags: review?(rcampbell) → review+
in the future, you should direct a review towards Mihai Sucan when making any console-related changes. Thanks!
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 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.
Attachment #704919 - Flags: review?(dteller)
Attachment #704919 - Flags: review+
bsmedberg, are you ready to land this?  It would be really nice if we could land bug 819000 on m-c.
Depends on: 852220
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?
Flags: needinfo?
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.
Flags: needinfo?
You need to log in before you can comment on or make changes to this bug.