ContentChild's nsIConsoleListener should have threadsafe addref/release methods

RESOLVED FIXED in Firefox 21

Status

()

Core
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: bsmedberg)

Tracking

({addon-compat, dev-doc-needed})

Trunk
mozilla21
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 fixed)

Details

Attachments

(5 attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
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.
Attachment #702956 - Flags: review?(benjamin)
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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-
(Reporter)

Comment 4

5 years ago
(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.
(Reporter)

Updated

5 years ago
Attachment #702956 - Flags: review- → review?(benjamin)
(Reporter)

Updated

5 years ago
Summary: ContentChild's nsIConsoleListener should be threadsafe → ContentChild's nsIConsoleListener should have threadsafe addref/release methods
(Assignee)

Comment 5

5 years ago
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.
Attachment #703051 - Flags: review?(justin.lebar+bug)
(Reporter)

Comment 6

5 years ago
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+
(Reporter)

Comment 7

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

Whoops, wrong patch.
Attachment #702956 - Attachment is obsolete: true
Attachment #702956 - Flags: review+
(Reporter)

Updated

5 years ago
Attachment #703051 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 8

5 years ago
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?
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
Attachment #703922 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

5 years ago
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.
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.
(Reporter)

Comment 12

5 years ago
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.
(Assignee)

Comment 13

5 years ago
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?
(Reporter)

Comment 14

5 years ago
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?
(Reporter)

Updated

5 years ago
Attachment #702956 - Attachment is obsolete: false
(Assignee)

Updated

5 years ago
Attachment #702956 - Flags: review?(benjamin) → review+
(Assignee)

Comment 15

5 years ago
Created attachment 704919 [details] [diff] [review]
Fix browser_Deprecated.js, rev. 1
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+
(Reporter)

Comment 17

5 years ago
https://hg.mozilla.org/releases/mozilla-b2g18/rev/57fc020315a0
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)
(Assignee)

Comment 19

5 years ago
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!
(Reporter)

Comment 22

5 years ago
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+
(Reporter)

Comment 24

5 years ago
bsmedberg, are you ready to land this?  It would be really nice if we could land bug 819000 on m-c.
Blocks: 835809
(Assignee)

Comment 25

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/507d85ab5807
https://hg.mozilla.org/mozilla-central/rev/5c5b63581fc2
https://hg.mozilla.org/mozilla-central/rev/f8b0ebdfe845
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → fixed
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → fixed

Updated

5 years ago
Depends on: 852220
Keywords: dev-doc-needed
Keywords: addon-compat

Comment 27

5 years ago
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?
(Assignee)

Comment 28

5 years ago
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.