123049 - AppendChange() includes frames that can be destroyed on the fly, causes crash

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[kinmoz]

Not sure exactly who to give this to, so I'm cc'ing some layout heavy-hitters
and assigning it to myself ...

This bug was originally reported as bug 117141 (Composer crashes when loading
pages with an HTML Select element [@FrameManager::GetPrimaryFrameFor]), for
which I checked in a work around.

The basic problem is that nsStyleChangeList::AppendChange() does not deal
with the fact that anonymous frames can point to either an anonymous content
node, or the content node for a parent frame.

Should two or more sibling anonymous content frames require a frame change,
we can get into trouble if we push an anonymous content node on the changelist,
and then push the parent content node on the list, as the result of a
sibling frame who's content pointer pointed to the parent content node.

nsCSSFrameConstructor::ProcessRestyledFrames() processes the changelist
backwards, which means it would process the parent content node first, so
as a result, we end up blowing away the entire frame hierarchy including
the anonymous content frames and all their anonymous content. Leaving
deleted nodes in the change list that will be processed.

When they do get processed we usually crash some place like
GetPrimaryFrameFor().

I'll attatch a JS test case that demostrates this crash rather easily
in the browser and viewer. It's probably a good idea to read over
the comments dbaron and I made in bug 117141 for more details.

A stack trace for the crash looks something like this:


FrameManager::GetPrimaryFrameFor(FrameManager * const 0x03d4e118, nsIContent *
0x03ab4b68, nsIFrame * * 0x0012ccec) line 645 + 33 bytes
PresShell::GetPrimaryFrameFor(const PresShell * const 0x03890b08, nsIContent *
0x03ab4b68, nsIFrame * * 0x0012ccec) line 5453 + 32 bytes
nsCSSFrameConstructor::RecreateFramesForContent(nsIPresContext * 0x03951220,
nsIContent * 0x03ab4b68, int 0, nsIStyleRule * 0x00000000, nsIStyleContext *
0x00000000) line 11834
nsCSSFrameConstructor::ProcessRestyledFrames(nsCSSFrameConstructor * const
0x03d53160, nsStyleChangeList & {...}, nsIPresContext * 0x03951220) line 10012
nsCSSFrameConstructor::AttributeChanged(nsCSSFrameConstructor * const
0x03d53160, nsIPresContext * 0x03951220, nsIContent * 0x03c8fbb0, int 0, nsIAtom
* 0x0158a710, int 1, int 3) line 10558
StyleSetImpl::AttributeChanged(StyleSetImpl * const 0x03622a30, nsIPresContext *
0x03951220, nsIContent * 0x03c8fbb0, int 0, nsIAtom * 0x0158a710, int 1, int -1)
line 1495
PresShell::AttributeChanged(PresShell * const 0x03890b10, nsIDocument *
0x03cf6028, nsIContent * 0x03c8fbb0, int 0, nsIAtom * 0x0158a710, int 1, int -1)
line 5121 + 61 bytes
nsDocument::AttributeChanged(nsDocument * const 0x03cf6028, nsIContent *
0x03c8fbb0, int 0, nsIAtom * 0x0158a710, int 1, int -1) line 1970 + 36 bytes
nsHTMLDocument::AttributeChanged(nsHTMLDocument * const 0x03cf6028, nsIContent *
0x03c8fbb0, int 0, nsIAtom * 0x0158a710, int 1, int -1) line 1463
nsGenericHTMLElement::SetAttr(nsGenericHTMLElement * const 0x03c8fbb0, int 0,
nsIAtom * 0x0158a710, const nsAString & {...}, int 1) line 1663
nsGenericHTMLElement::SetFormControlAttribute(nsIForm * 0x00000000, int 0,
nsIAtom * 0x0158a710, const nsAString & {...}, int 1) line 4142 + 28 bytes
nsGenericHTMLContainerFormElement::SetAttr(nsGenericHTMLContainerFormElement *
const 0x03c8fbb0, int 0, nsIAtom * 0x0158a710, const nsAString & {...}, int 1)
line 4180
nsGenericHTMLElement::SetAttr(nsGenericHTMLElement * const 0x03c8fbb0,
nsINodeInfo * 0x039519d8, const nsAString & {...}, int 1) line 1685 + 33 bytes
nsGenericHTMLContainerFormElement::SetAttr(nsGenericHTMLContainerFormElement *
const 0x03c8fbb0, nsINodeInfo * 0x039519d8, const nsAString & {...}, int 1) line
4188
nsGenericElement::SetAttribute(nsGenericElement * const 0x03c8fbb0, const
nsAString & {...}, const nsAString & {...}) line 954 + 27 bytes
nsGenericHTMLElement::SetAttribute(nsGenericHTMLElement * const 0x03c8fbb0,
const nsAString & {...}, const nsAString & {...}) line 109
nsHTMLSelectElement::SetAttribute(nsHTMLSelectElement * const 0x03c8fbb0, const
nsAString & {...}, const nsAString & {...}) line 140 + 20 bytes
XPTC_InvokeByIndex(nsISupports * 0x03c8fbe0, unsigned int 30, unsigned int 2,
nsXPTCVariant * 0x0012d5f8) line 106
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode
CALL_METHOD) line 1998 + 42 bytes
XPC_WN_CallMethod(JSContext * 0x0361a768, JSObject * 0x02d18d40, unsigned int 2,
long * 0x03cfad4c, long * 0x0012d8d4) line 1266 + 14 bytes
js_Invoke(JSContext * 0x0361a768, unsigned int 2, unsigned int 0) line 832 + 23
bytes
js_Interpret(JSContext * 0x0361a768, long * 0x0012e6c4) line 2800 + 15 bytes
js_Invoke(JSContext * 0x0361a768, unsigned int 1, unsigned int 2) line 849 + 13
bytes
js_InternalInvoke(JSContext * 0x0361a768, JSObject * 0x02d17520, long 47287432,
unsigned int 0, unsigned int 1, long * 0x0012e91c, long * 0x0012e7ec) line 924 +
20 bytes
JS_CallFunctionValue(JSContext * 0x0361a768, JSObject * 0x02d17520, long
47287432, unsigned int 1, long * 0x0012e91c, long * 0x0012e7ec) line 3405 + 31 bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x02fb10c8, void * 0x02d17520,
void * 0x02d18c88, unsigned int 1, void * 0x0012e91c, int * 0x0012e920, int 0)
line 1016 + 33 bytes
nsJSEventListener::HandleEvent(nsJSEventListener * const 0x03d2bc00, nsIDOMEvent
* 0x03c4fb70) line 180 + 77 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x03949518,
nsIDOMEvent * 0x03c4fb70, nsIDOMEventTarget * 0x0393ab70, unsigned int 4,
unsigned int 7) line 1205 + 20 bytes
nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x03d2bb88,
nsIPresContext * 0x03951220, nsEvent * 0x0012f48c, nsIDOMEvent * * 0x0012f124,
nsIDOMEventTarget * 0x0393ab70, unsigned int 7, nsEventStatus * 0x0012f828) line
1373 + 36 bytes
nsGenericElement::HandleDOMEvent(nsGenericElement * const 0x03d2b838,
nsIPresContext * 0x03951220, nsEvent * 0x0012f48c, nsIDOMEvent * * 0x0012f124,
unsigned int 1, nsEventStatus * 0x0012f828) line 1652
nsHTMLInputElement::HandleDOMEvent(nsHTMLInputElement * const 0x03d2b838,
nsIPresContext * 0x03951220, nsEvent * 0x0012f48c, nsIDOMEvent * * 0x00000000,
unsigned int 1, nsEventStatus * 0x0012f828) line 1190 + 29 bytes
PresShell::HandleEventInternal(nsEvent * 0x0012f48c, nsIView * 0x00000000,
unsigned int 1, nsEventStatus * 0x0012f828) line 6000 + 44 bytes
PresShell::HandleEventWithTarget(PresShell * const 0x03890b08, nsEvent *
0x0012f48c, nsIFrame * 0x03b5819c, nsIContent * 0x03d2b838, unsigned int 1,
nsEventStatus * 0x0012f828) line 5969 + 22 bytes
nsEventStateManager::CheckForAndDispatchClick(nsEventStateManager * const
0x0394a128, nsIPresContext * 0x03951220, nsMouseEvent * 0x0012f92c,
nsEventStatus * 0x0012f828) line 2463 + 63 bytes
nsEventStateManager::PostHandleEvent(nsEventStateManager * const 0x0394a130,
nsIPresContext * 0x03951220, nsEvent * 0x0012f92c, nsIFrame * 0x03b5819c,
nsEventStatus * 0x0012f828, nsIView * 0x0394da90) line 1544 + 28 bytes
PresShell::HandleEventInternal(nsEvent * 0x0012f92c, nsIView * 0x0394da90,
unsigned int 1, nsEventStatus * 0x0012f828) line 6020 + 43 bytes
PresShell::HandleEvent(PresShell * const 0x03890b0c, nsIView * 0x0394da90,
nsGUIEvent * 0x0012f92c, nsEventStatus * 0x0012f828, int 0, int & 1) line 5923 +
25 bytes
nsView::HandleEvent(nsView * const 0x0394da90, nsGUIEvent * 0x0012f92c, unsigned
int 0, nsEventStatus * 0x0012f828, int 0, int & 1) line 390
nsView::HandleEvent(nsView * const 0x03d06380, nsGUIEvent * 0x0012f92c, unsigned
int 0, nsEventStatus * 0x0012f828, int 0, int & 1) line 347
nsView::HandleEvent(nsView * const 0x03d8f228, nsGUIEvent * 0x0012f92c, unsigned
int 0, nsEventStatus * 0x0012f828, int 1, int & 1) line 347
nsViewManager::DispatchEvent(nsViewManager * const 0x03956ab8, nsGUIEvent *
0x0012f92c, nsEventStatus * 0x0012f828) line 1900
HandleEvent(nsGUIEvent * 0x0012f92c) line 83
nsWindow::DispatchEvent(nsWindow * const 0x0392c4fc, nsGUIEvent * 0x0012f92c,
nsEventStatus & nsEventStatus_eIgnore) line 850 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f92c) line 871
nsWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 4527 +
21 bytes
ChildWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 4779
nsWindow::ProcessMessage(unsigned int 514, unsigned int 0, long 1507413, long *
0x0012fd20) line 3419 + 24 bytes
nsWindow::WindowProc(HWND__ * 0x000b027e, unsigned int 514, unsigned int 0, long
1507413) line 1115 + 27 bytes
USER32! 77e12e98()
USER32! 77e130e0()
USER32! 77e15824()
nsAppShellService::Run(nsAppShellService * const 0x010d71a8) line 308
main1(int 1, char * * 0x003c7380, nsISupports * 0x00000000) line 1285 + 32 bytes
main(int 1, char * * 0x003c7380) line 1625 + 37 bytes
mainCRTStartup() line 338 + 17 bytes

Comment 1

[kinmoz]

Attachment: JS Test Case (Crashes the browser/viewer)

Comment 2

[Chris Petersen]

Changing QA Contact

Comment 3

[rbs]

This bug rings a bell like in bug 79866. We fixed it over there by separation.

Comment 4

[rbs]

>nsCSSFrameConstructor::ProcessRestyledFrames() processes the changelist
>backwards, which means it would process the parent content node first, so
>as a result, we end up blowing away the entire frame hierarchy including
>the anonymous content frames and all their anonymous content. Leaving
>deleted nodes in the change list that will be processed.

An alternative way (simple and very low-risk I might add) to reliably solve the
problem could be to change ProcessRestyledFrames() to proceed as follows:

1. before kicking off: for each anonymous content frame, register a dummy
   property in the frame manager.

2. during processing, skip those (dead) anonymous frames for whom their property
   doesn't exist in the frame manager anymore.

[The basic idea is that since frames notify the frame manager when they are
going away, and the frame manager removes any property that has been registered
for them, you would then tell if a frame is still alive or not by checking the
property that you _know_ should have been there. And programmatically, the
property can be an nsAutoVoidArray initialized with the frames, and for which
the destroy function simply consists in removing the frame from the void array.]

Comment 5

[Brendan Eich [:brendan]]

Should this be fixed in 0.9.9?

/be

Comment 6

[greer]

This one is a topcrasher on M098, with a few incidents on the Trunk, so I think 
the answer to Brendan's previous question is "yes".

Comment 7

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

This bug is unlikely to be the topcrasher on the trunk.  Please don't make this
bug any more confusing than it already is.  The topcrash on the trunk is likely
related to the bug where OBJECT-replacement causes frames to be readded to the
primary frame map during destruction, or a similar bug with block-in-inline
stuff.

Comment 8

[Marek Z. Jeziorek]

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

Comment 9

[Jaime Rodriguez, Jr.]

I am unable to reproduce this crash on a 20021008 1.0 branch build. Do we have
any recent Talkback data on this issue?

Comment 10

[Judson Valeski]

I was able to repro this on today's win2k trunk build.

Comment 11

[rbs]

Re-assigning to myself. Having seen tons of related bugs, I am going to attach
patch that implements the idea I noted in comment 4. The patch is likely going
to fix those numerous crasher bugs with
"nsCSSFrameConstructor::ProcessRestyledFrames" in the stack trace (not 100%
certain, didn't read all, please verify). Did a quick search in bugzilla and saw:

bug 114513 N620 crash [@ 0x003b009c - nsCSSFrameConstructor::Proces...
bug 124596 crashing Mozilla is child's play [HandleScrollEvent]
bug 141590 M1BR crash [@0x00000000 - IsCanvasFrame][@ 0x032eb41c -ns...
bug 166205 URL causes Mozilla to crash. Crash is severe, even XP's t...
bug 187548 Editing or saving page with some given CSS crashes Mozilla
bug 187671 {ib}crash in nsCSSFrameConstructor::StyleChangeReflow
bug 187890 Crash scrolling page scottcollins.net - Trunk [@ IsCanvas...
bug 133219 Browser crash when selecting alternate style sheet; M11A ...
bug 166596 Crash saving page with border padding issues [@ nsStyleC...
bug 123049 AppendChange() doesn't handle anonymous content frames, c...
bug 154797 {ib}[RR]Browser crashes inserting linked stylesheet
bug 166569 [RR/RCF]perhaps closing a window using a menu (or not usi...

Comment 12

[rbs]

Attachment: patch

Fixes this bug and related bugs that I can test so far.

Note regarding the patch: the |if (nsLayoutAtoms::changeListProperty ==
aPropertyName)| isn't necessary since there is only one consumer at the moment.
But it highlights that if people want to be notified in other cirumstances they
can just register the same function and add their own |if| there.

Comment 13

[rbs]

Maybe I will change the name to:
s/NotifyFrameDestroyed/RemoveFramePropertyCallback/

Comment 14

[Boris Zbarsky [:bzbarsky]]

This patch does fix the crash in bug 187890, eg, but it also triggers the
following assertion:

###!!! ASSERTION: unexpected null param: 'aPropertyName && aFrame', file
/home/bzbarsky/mozilla/profile/mozilla/layout/html/base/src/nsFrameManager.cpp,
line 2536

So the issue is that anonymous content frames can cause a reframe of the parent
content node and don't stop the processing in ReResolveStyleContext as they
should when this happens?

Perhaps AppendChange should be smarter about screening our multiple changes
targeted at the same content node?

Comment 15

[rbs]

Curious, I don't get the assertion.
|aPropertyName| is always passed as a hard nsLayoutAtoms::changeListProperty.
|aFrame| comes straight from the change list and so it can't be null, or... can it?
Any clues? Might be other changes in your tree?

Since the patch also fixes other issues (e.g., bug 154797, bug 154797, bug
187671, etc) where it is not clear where/when anonymous frames come in, seems
that it is easier/simpler to avoid these headaches once for all. The root cause
of the crashes are those frames in the change list that get destroyed due to
some ripple effect along the way.

Comment 16

[rbs]

OK, found out, aFrame can be null in the change list:
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsFrameManager.cpp#1899

Need to update the patch with the appropriate null checks.

Comment 17

[Boris Zbarsky [:bzbarsky]]

The cases where anonymous frames don't come in all have {ib} situations.... I'm
looking into it right now.

Comment 18

[rbs]

Attachment: patch with null checks

Comment 19

[rbs]

Comment on attachment 110845 [details] [diff] [review]
patch with null checks

Migrating the earlier requests (bz, feel free to pick either since you are
looking).

Comment 20

[Boris Zbarsky [:bzbarsky]]

> where it is not clear where/when anonymous frames come in

They do not.  Those bugs are caused by the fact that reframing a frame that's
part of an {ib} split actually reframes its _parent_.  Thus any siblings of it
that were in the change list end up dead...

That issue should be fixed by moving {ib} handling to reflow.

Out of curiousity, have you run any perf tests for silly things like changing
.style.color 10000 times on something with your patch?  Is there any perf impact?

Comment 21

[rbs]

I haven't done any perf measurements. I was mostly interested in the underlying
logic that led to the crashers. There might be a little perf impact, but
hopefully it is going to be noise compared to the titanic task that goes into
ReResolve which populated the change list in the first place.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Ok... the more I think about this, the more convinced I get that we do actually
want something like this patch.  Trying to make sure that the changes are
properly ordered in the list and don't stomp on each other seems like it would
be too painful (code-readability-wise, and possibly perf-wise).

One thing that concerns me in the use of the processQueue array is that it makes
this code O(N^2) in the number of frames (due to the indexOf calls on every
iteration).  Would it be better, perhaps, to do the following?

1)  go through and set the frame property on all frames in the list
2)  go through and do whatever change is needed on all frames in the list with
    the property
3)  go through and clear the frame property on all the frames.

This should at least be linear in the number of changes (though possibly more
expensive in the "average" case of only a few changes  :( ).

One other thing I'm wondering is why we process the change hints from the end
forward...  is there a good reason?  If so, we should comment it.  If not, would
changing the order be better or not?  (Obviously not a central issue for this bug).

Comment 23

[rbs]

bz, don't concern yourself that much with the perf impact in this situation
because, practically, the size of the list is often O(1), i.e., in tens or less.
Will be noise with todays's CPUs. Remember premature optimization? The cause of
more harm than good, especially in this complex code. Let's instead whatch the
Tp radars to see how they go. My gut feeling is that there won't be cause for
another iteration that will obfuscate the code even more.

(Not sure why the previous code walked the list backward. I have documented the
reason of the backward walk that I added.)

Comment 24

[Boris Zbarsky [:bzbarsky]]

rbs, the change list will include nearly everything on the page for things like
stylesheet changes (to alternate sheets), stylesheet loads (once the css loader
does not block the parser, these could come in after lots of layout is done),
and skin switches (if we ever bring back dynamic skin switching).

Even just the alternate stylesheet switching case worries me....  Could you try
it on a few largish pages (eg http://www.w3.org/Style/CSS/) to see how it looks?

But I agree that what I propose is not really that much better and is somewhat
less clear... :(

Comment 25

[rbs]

Note that what I am meaning is that the cost of other things
(reresolve/delete/reconstruct the frame tree) is often going to absorb the
little extra in this case. Of course, optimization is generally a good thing.

Browsing with a |printf| and going to the W3C CSS page, I see: 
processQueue count:1
processQueue count:1
processQueue count:1
processQueue count:1
processQueue count:1
processQueue count:1
processQueue count:1
processQueue count:105  <-- up tick when switching to an alternate style to
processQueue count:1        destroy/reresolve/reconstruct.
processQueue count:1
processQueue count:1
processQueue count:1

Comment 26

[rbs]

Actually, just thought that the queue could be turned to a hash if the lookup
perf proves critical later on.

Comment 27

[rbs]

> That issue should be fixed by moving {ib} handling to reflow.

I seem to remember that it used to be in reflow and kipp moved it to the frame
construction code, so it isn't clear how effective/beneficial an u-turn will be.
Got some handy bug numbers about the proposed change for future references?

===
Updating title to reflect the full extend of the problem (which includes
anonymous content frames as well as non-anonymous ones). The patch isn't quite
what is outlined in comment 4 since it is queueing the whole lot (that's why it
fixes the other bugs).

Comment 28

[Boris Zbarsky [:bzbarsky]]

bug 142585 and its dependencies... the code was moved to frame construction by
waterson, and at this point pretty much everyone who has thought about it
(including waterson) thinks that a u-turn is needed.  Just search for bugs with
"{ib}" in the summary... ;)

Comment 29

[rbs]

Saw it. Seems like a simpler/easier/better proposal for that {ib}.

Comment 30

[rbs]

Attachment: more efficient patch that makes the most of the frame manager hashtable

Avoid |IndexOf()| with some voodoo. The patch capitalizes on the frame manager
hashtable and uses |NS_IFRAME_MGR_REMOVE_PROP| to embed the cleanup along the
way.

Comment 31

[Boris Zbarsky [:bzbarsky]]

I thought about suggesting that... the problem is that the same frame can be in
the list multiple times with several different hints, as far as I can tell (as
long as none of those hints are nsChangeHint_ReconstructFrame)... 

Perhaps the code in nsStyleChangeList::AppendChange should just coalesce all
hits, not just nsChangeHint_ReconstructFrame?  That still makes the change list
construction O(N^2), but would make for much cleaner arch elsewhere (eg we could
use the "more efficient patch" as well as having a nice invariant about the
change list....).

Comment 32

[rbs]

> the problem is that the same frame can be in
> the list multiple times with several different hints, as far as I can tell

Indeed, it seems that it was the price tag to keep AppendChange() linear. But
since the poperty hashtable overwrites and only keep a single reference (the
hint doesn't matter), it isn't strictly a "problem" per see. The earlier
alternative to filter out these duplicates was that IndexOf :-)

> Perhaps the code in nsStyleChangeList::AppendChange should just coalesce

Yep, might need to redesign the data structure itself with a coalesce in mind to
make it more efficient, e.g., sorted with respect to hints or something. Looks
like some food for thoughts here.

Comment 33

[Boris Zbarsky [:bzbarsky]]

> it isn't strictly a "problem" per see

Yes, it is.  If a frame is in the list with multiple hints, we will only process
_one_ of the hints with that patch (for the other instances, the property will
be gone).

Comment 34

[rbs]

Good catch... Need some extra voodoo, or perhaps a return to the safer IndexOf :-)

Comment 35

[rbs]

Comment on attachment 110845 [details] [diff] [review]
patch with null checks

OK, back to the safer IndexOf()... leaving the other patch until such a time
(if any) where the change list is made to hold only unique entries. Premature
optimzation? Beware.

Comment 36

[rbs]

Comment on attachment 110845 [details] [diff] [review]
patch with null checks

bz, r= for this crasher to let it bake in the tree a little bit? You have
already looked at the patch in great detail.

BTW, I could iterate on the "more efficient" patch to:

1) not set the option to remove the prop in the processing loop
2) then cleanup at the end, i.e., another loop over the change list with the
option to now effectively remove the props that are still there.

But I have been favoring keeping the code simpler in this round. What do you
think?

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 110845 [details] [diff] [review]
patch with null checks

r=me, if there is no obvious perf impact on the alternate sheet switching thing
I mentioned....

Comment 38

[kinmoz]

rbs, I know you were saying earlier not to worry too much about the perf impact
of this, but I'm just curious if you've noticed any perf impact when loading a
large doc (with tables and form elements) in composer? In composer, I believe
the entire doc is loaded, and then some override stylesheets are applied.

Comment 39

[rbs]

Attachment: alternate high performance patch

I am not "perceiving" a significant perf impact -- there are the (usual) lags
when opening the editor. I tried attachment 110876 [details] (warning: large table with
forms, from bug 141572) ang got:

[...]
processQueue count: 1
processQueue count: 1
processQueue count: 1399 <-- uptick probably when override stylesheets kick in
processQueue count: 180
processQueue count: 1
processQueue count: 1
[...]

Since that testcase is already slow by itself, any extra thing is shadowed.

However, since the perf issue has been recurring from bz and now yourself, you
may be smelling something fishy. So I have iterated on the more efficient
patch. Perhaps your apprehensions warrant switching to this one instead?

[The bottom line is that it is necessary to be able to know when frames die
along the way. So using markers such as I am doing seems inevitable (i.e., I
can't think of another simpler, robust and secure way to address these
crashers).]

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 111671 [details] [diff] [review]
alternate high performance patch

I think I like this more....

But we should really de-pseudoCOMify nsStyleChangeList (separate bug).

Comment 41

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

Comment on attachment 111671 [details] [diff] [review]
alternate high performance patch

sr=dbaron, although it might be good to add a comment briefly summarizing
comment 31.

Comment 42

[rbs]

Checked in with updated comment. Watching Tinderbox didn't raise an audible alarm.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Something I missed.. the comment on the property in nsLayoutAtomList seems to
have gotten lost (what type the prop is).  Would you mind putting that back?
(and r/sr=me on that change).

Comment 44

[rbs]

Checked in a comment to note down that it is |void*| -- it is a dummy property
which just creates a null entry in the frame manager's hashtable.

+      frameManager->SetFrameProperty(changeData->mFrame,
+        nsLayoutAtoms::changeListProperty, nsnull, nsnull);

Comment 45

[Jesse Ruderman]

Crashtest added as part of http://hg.mozilla.org/mozilla-central/rev/54417ebbaea2