Closed Bug 136704 Opened 22 years ago Closed 22 years ago

XBL anonymous content needs to be removed from the frame manager's undisplayed map when it is destroyed

Categories

(Core :: XBL, defect, P1)

Product:
Core
Component:
XBL
Platform:
x86
Windows 2000
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.1alpha

People

(Reporter: attinasi, Assigned: dbaron)

References

Details

(Keywords: topembed)

Attachments

(2 files, 2 obsolete files)

add missed PresShell changes; fix comment
13.54 KB, patch
attinasi
: review+
waterson
: superreview+
Details | Diff | Splinter Review
possible workaround for crash
6.85 KB, patch
attinasi
: review+
waterson
: superreview+
Details | Diff | Splinter Review 
This is an offshoot of bug 118014, where I saw that a stale anonymous content
element from XBL is being left in the undisplayed map. The element is being
destroyed in nsBindingManager::ChangeDocumentFor.

The temporary workaround was to clear the undisplayed content map when style
data is reconstructed, but this is not right (the undisplayed map needs to be
used to reresolve the undisplayed elements in case they should now be displayed).

When this notification is worked out, and we are sure that these anonymous
content elements are not being left in the undisplayed map, we should also
remove the call to frameManager->ClearUndisplayedContentMap() in
nsPresShell::ReconstructStyleData
Do you have an exact stack for the way the content node was destroyed?
It seems somewhat like SetDocument(null) time (which is triggered by
DocumentViewerImpl::Close, not Destroy) is too early to be doing:

561 SetInsertionParent(aContent, nsnull);
562 SetContentListFor(aContent, nsnull);
563 SetAnonymousNodesFor(aContent, nsnull);

since we might still want to reflow or paint.  I'm assuming it's the last that's
causing the problem here, though.  It seems like this might be a case of the
content nodes being released too early rather than a missing ContentRemoved
notification.
(That suggests another possible workaround for the problem -- clearing the
undisplayed content map from DocumentViewerImpl::Close, which I don't think
would have any bad side effects since scripts are turned off there.  Might that
also fix bug 118014?)
OK, I found a reproducable way to hit the "node in map twice" assertions (by
opening the XP (i.e., Unix) file picker from within the code I'm working on for
bug 137331).  This led me to a likely cause of the problem, which is that the
XBL resource loader determines whether it's already sent a ContentInserted
notification by seeing if an element has a frame.  It also needs to check the
undisplayed map, or it will end up sending multiple notifications.  This also
led me to notice a bug in a rarely-used function in the undisplayed map (and I
*think* the two existing callers want the function to work right, and they're
both essentially a performance optimization, so it might not have been noticed).

Patch to be attached, although I think it introduces a dependency that I don't
want (xbl on nsIStyleContext), so I should find a way to rewrite it without the
dependency.
Attached patch patch (obsolete) — — Splinter Review 
Oh, and hyatt added |GetUndisplayedContent| in revision 1.74 of nsFrameManager,
while fixing bug 78695 -- it wasn't there from the beginning.
Marc, could you test to see if this fixes the "reproducable" testcase from bug
118014 (after removing the ClearUndisplayedContentMap too)?  (I never could
figure out how to reproduce it.)
So I started thinking that maybe the incorrect GetUndisplayedContent could have
caused a bug -- after all, it would be returning the wrong style context. 
However, it couldn't, and here's why:  The one place it was used was in
nsCSSFrameConstructor::AttributeChanged, in the optimization for changes to the
style attribute.  It was used only in the case where the element in question
didn't have a primary frame.  The incorrectess in the current implementation
would have caused the function to return the style context for the first (in the
map) undisplayed child of a node rather than the style context for the node
itself.  However, since it was only called for nodes that had no primary frame,
their children would never have been processed, so it would always return null
in the case where it was used, simply disabling the optimization of using the
style context from the undisplayed map rather than reresolving a new one.

However, I think that in its fixed form, AttributeChanged shouldn't need the
extra code if the Undisplayed map doesn't have the style context, because that
means the element in question is a descendant of a 'display: none' frame and has
no style context.
Taking
Assignee: hyatt → dbaron
Target Milestone: --- → mozilla1.0
Here's the problem:

You have a parent with binding A, and a display: none child also with binding 
A.  Both are observing the load of binding A.  In the resource loader loop, 
the parent gets notified first, and he constructs frames, adding the child to 
the undisplayed map.  Then the child gets notified that binding A is loaded, 
and the GPFF check was supposed to stop a double frame construction.  The 
undisplayed map check was stop a double map insertion.  So this is a good 
patch.

You also want to patch nsXBLService.cpp.  It has a check as well that could 
conceivably hit the same problem.
Attached patch revised patch (obsolete) — — Splinter Review 

        Attachment #79367 -
        Attachment is obsolete: true

    
    

    
  
> +        // If |content| is (in addition to having binding |mBinding)

There's a missing "|" in that comment (which appears in two files... is there a
bug on reducing the code duplication?).

    
    

    
  
I see the assertions in the testcase for bug 138120.
Status: NEW → ASSIGNED
Priority: -- → P1

    
    

    
  
Comment on attachment 80527 [details] [diff] [review]
add missed PresShell changes; fix comment

r=/sr=waterson

        Attachment #80527 -
        Flags: superreview+

    
    

    
  
I filed bug 141303 and bug 141304 on the XXX comments at the end of the patch.

    
    

    
  
Comment on attachment 80527 [details] [diff] [review]
add missed PresShell changes; fix comment

r=attinasi, but are you sure we don't need to call
frameManager->ClearUndisplayedContentMap in the case where we are rebuilding
the rule tree too (we used to, before my hack patch).

Also, I don't really understand this assertion:
+			   NS_ASSERTION(!parentFrame,
+					"parent frame but no child frame "
+					"or undisplayed entry");
Should the GetParentFrameFor be DEBUG only since it is only used in the
ASSERTION?

        Attachment #80527 -
        Flags: review+

    
    

    
  
> are you sure we don't need to call
> frameManager->ClearUndisplayedContentMap in the case where we are rebuilding
> the rule tree too (we used to, before my hack patch).

Yes.  See bug 136728.

> Also, I don't really understand this assertion:
> +	
	   NS_ASSERTION(!parentFrame,
> +	
			"parent frame but no child frame "
> +	
			"or undisplayed entry");
> Should the GetParentFrameFor be DEBUG only since it is only used in the
> ASSERTION?

It is DEBUG-only -- I added |#ifdef DEBUG| around a good bit of code surrounding
that (because I changed code that did something to an assertion), although that
might be hard to tell from the diff.  The point of the assertion is that if we
have a parent frame there should always be either a child frame or an
undisplayed content map entry.  We depend on that invariant for dynamic style
changes (like the ones in bug 136728) to work.

    
    

    
  
oh, duh, I see the #ifdef DEBUG now. Sorry. Thanks for the clarification.

    
    

    
  
Fix checked in to trunk 2002-04-30 17:36 PDT.

    
    

    
  
For the record, email I sent about this bug:

 * It undoes Marc's workaround for bug 118014 and removes the
   ClearUndisplayedMap completely to fix 136728 (see the latter bug for
   why)
 * Fixes the XBL code to not send multiple ContentInserted notifications
   for the same content, and explains why the code in question needs to 
   be there (as hyatt explained to me on IRC), by using
   GetUndisplayedContent.  (hyatt didn't seem to mind the dependency,
   although I don't really like it, and I'm really not sure what a   
   "good" way around it would be.  suggestions?)
 * Fixes GetUndisplayedContent to work, since hyatt didn't understand
   the undisplayed map when he wrote it.
 * Fixes the code that was working around GetUndisplayedContent not
   working (in nsCSSFrameConstructor::AttributeChanged) and changes it
   to an assertion (which I've still seen fire in the testcase on one 
   bug, and I'll need to investigate, but it really shouldn't happen).


    
    

    
  
Hmmm.  This seems to have brought back the original crash, although perhaps in
lower numbers.  I should try a workaround to block multiple removes in the
undisplayed map.

    
    

    
  


  
I removed the SetUndisplayedPseudoIn stuff -- it's almost useless, and we need
to find a better way to handle the one thing that it does manage to do, since
we don't put pseudos in the map if ProbePseudoStyleContext returns null.
Target Milestone: mozilla1.0 → mozilla1.1alpha

    
    

    
  
Comment on attachment 83060 [details] [diff] [review]
possible workaround for crash

r=attinasi

        Attachment #83060 -
        Flags: review+

    
    

    
  
Comment on attachment 83060 [details] [diff] [review]
possible workaround for crash

sr=waterson

        Attachment #83060 -
        Flags: superreview+

    
    

    
  
Above patch checked in to trunk 2002-05-16 06:30 PDT.

    
    

    
  
So it turns out my suspicion in comment 2 and comment 3 was actually correct --
see the fix for bug 145737.

    
    

    
  
The last reports of RuleProcessorData::RuleProcessorData crashes in talkback
were on 5-25, so I think bug 145737 was the final fix needed to fix bug 118014.
 Marking as fixed.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED

    
    

    
  
batch: adding topembed per Gecko2 document
http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html
Keywords: topembed

    
  
Blocks: grouper

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: