Add a way to unmark all the listeners in black documents

RESOLVED FIXED in mozilla12

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

12 Branch
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 591019 [details] [diff] [review]
patch
Attachment #591019 - Flags: review?(continuation)
(Assignee)

Comment 1

5 years ago
Created attachment 591025 [details] [diff] [review]
this might even compile
Attachment #591025 - Flags: review?(continuation)

Updated

5 years ago
Attachment #591019 - Attachment is obsolete: true
Attachment #591019 - Flags: review?(continuation)
OS: Linux → All
Hardware: x86_64 → All
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?
Attachment #591025 - Flags: review?(continuation) → review+
(Assignee)

Comment 3

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

Comment 4

5 years ago
Created attachment 591777 [details] [diff] [review]
patch
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?
(Assignee)

Comment 6

5 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.