Closed Bug 136704 Opened 18 years ago Closed 18 years ago
XBL anonymous content needs to be removed from the frame manager's undisplayed map when it is destroyed
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.
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.
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.
> + // 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+
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: 18 years ago
Resolution: --- → FIXED
batch: adding topembed per Gecko2 document http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html
You need to log in before you can comment on or make changes to this bug.