Last Comment Bug 720630 - Add a way to unmark all the listeners in black documents
: Add a way to unmark all the listeners in black documents
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: mozilla12
Assigned To: Olli Pettay [:smaug] (high review load, please consider other reviewers)
:
Mentors:
Depends on: 720536
Blocks: 705582
  Show dependency treegraph
 
Reported: 2012-01-24 01:05 PST by Olli Pettay [:smaug] (high review load, please consider other reviewers)
Modified: 2012-01-26 13:19 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.94 KB, patch)
2012-01-24 01:05 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review
this might even compile (3.81 KB, patch)
2012-01-24 01:15 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
continuation: review+
Details | Diff | Review
patch (3.81 KB, patch)
2012-01-26 06:52 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review

Description Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-24 01:05:15 PST
Created attachment 591019 [details] [diff] [review]
patch
Comment 1 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-24 01:15:54 PST
Created attachment 591025 [details] [diff] [review]
this might even compile
Comment 2 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-01-25 14:18:19 PST
Comment on attachment 591025 [details] [diff] [review]
this might even compile

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

::: content/base/public/nsContentUtils.h
@@ +993,5 @@
>     */
>    static nsEventListenerManager* GetListenerManager(nsINode* aNode,
>                                                      bool aCreateIfNotFound);
>  
> +  static void UnmarkGrayJSListenersInCCGenerationDocuments(PRUint32 aGeneration);

That's quite a long name.  Too bad InCCGeneration wasn't something a little shorter.  Oh well. :)

::: content/base/src/nsContentUtils.cpp
@@ +3398,5 @@
> +  PRUint32* gen = static_cast<PRUint32*>(aArg);
> +  EventListenerManagerMapEntry* entry =
> +    static_cast<EventListenerManagerMapEntry*>(aEntry);
> +  if (entry) {
> +    nsINode* n = static_cast<nsINode*>(entry->mListenerManager->GetTarget());

The target keeps the listener manager alive, right?

@@ +3400,5 @@
> +    static_cast<EventListenerManagerMapEntry*>(aEntry);
> +  if (entry) {
> +    nsINode* n = static_cast<nsINode*>(entry->mListenerManager->GetTarget());
> +    if (n && n->IsInDoc() &&
> +        n->OwnerDoc()->GetMarkedCCGeneration() == nsCCUncollectableMarker::sGeneration) {

Should this be nsCCUncollectableMarker::InGeneration(n->OwnerDoc()->GetMarkedCCGeneration()) so you check that the generation is non-zero?
Comment 3 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-26 01:58:11 PST
(In reply to Andrew McCreight [:mccr8] from comment #2)
> 
> The target keeps the listener manager alive, right?
yes


> 
> @@ +3400,5 @@
> > +    static_cast<EventListenerManagerMapEntry*>(aEntry);
> > +  if (entry) {
> > +    nsINode* n = static_cast<nsINode*>(entry->mListenerManager->GetTarget());
> > +    if (n && n->IsInDoc() &&
> > +        n->OwnerDoc()->GetMarkedCCGeneration() == nsCCUncollectableMarker::sGeneration) {
> 
> Should this be
> nsCCUncollectableMarker::InGeneration(n->OwnerDoc()-
> >GetMarkedCCGeneration()) so you check that the generation is non-zero?
Ah, I guess I added this before adding the non cb version of generation check.
Comment 4 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-26 06:52:19 PST
Created attachment 591777 [details] [diff] [review]
patch
Comment 5 Daniel Holbert [:dholbert] 2012-01-26 12:18:16 PST
It looks like this landed:
  http://hg.mozilla.org/mozilla-central/rev/2bfef4ad7ad3

Should this bug be closed?

Also, this introduced this build warning:
> nsContentUtils.cpp:3397:13: warning: unused variable ‘gen’ [-Wunused-variable]
from this chunk:
> 3393 PLDHashOperator
> 3394 ListenerEnumerator(PLDHashTable* aTable, PLDHashEntryHdr* aEntry,
> 3395                    PRUint32 aNumber, void* aArg)
> 3396 {
> 3397   PRUint32* gen = static_cast<PRUint32*>(aArg);


|gen| is indeed unused in that method. Can we just drop it?
Comment 6 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-26 12:29:52 PST
https://hg.mozilla.org/mozilla-central/rev/2bfef4ad7ad3

I'm closing stuff once I know the tree is green.
And yes, that gen looks like a leftover. I'll fix it.

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