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

Status: RESOLVED FIXED

(Core :: XBL, defect, P1)

Description

[Marc Attinasi]

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

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Do you have an exact stack for the way the content node was destroyed?

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(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?)

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: patch

Oh, and hyatt added |GetUndisplayedContent| in revision 1.74 of nsFrameManager,
while fixing bug 78695 -- it wasn't there from the beginning.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.)

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Taking

Comment 9

[David Hyatt]

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.

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: revised patch

Comment 11

[Hixie (not reading bugmail)]

> +        // 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?).

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: add missed PresShell changes; fix comment

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I see the assertions in the testcase for bug 138120.

Comment 14

[Chris Waterson]

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

r=/sr=waterson

Comment 15

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

Comment 16

[Marc Attinasi]

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?

Comment 17

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

> 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.

Comment 18

[Marc Attinasi]

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

Comment 19

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

Comment 20

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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).

Comment 21

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 22

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: possible workaround for crash

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.

Comment 23

[Marc Attinasi]

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

r=attinasi

Comment 24

[Chris Waterson]

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

sr=waterson

Comment 25

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

Comment 26

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

Comment 27

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 28

[Marek Z. Jeziorek]

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