Bugzilla

Assignee

Comment 4

•

24 years ago

Thanks for the data.

Status: NEW → ASSIGNED

Assignee

Comment 5

•

24 years ago

Ugh. I cannot reproduce this on Mac or Windows using a CVS build from 5/14.

Looking at the code, it is very curious that we would be crashing with address
NULL in IsIncrementalDamageConstrained, where indicated. If I am reading it
correctly, the crash is on a dereference of a null 'target' pointer, but I
cannot see how it can ever be null!

    while (target)
    { // starting with the target's parent, scan for a text control
      nsIFrame *parent;
      target->GetParent(&parent); // THIS IS THE CRASH FROM FIRST STACk TRACE
      if ((nsIFrame*)this==parent || !parent) 
        break;
      nsCOMPtr<nsIAtom> frameType;
      parent->GetFrameType(getter_AddRefs(frameType));
      if (frameType)
      {
        if (nsLayoutAtoms::textInputFrame == frameType.get())
          return PR_TRUE; // damage is constrained to the text control innards
      }
      target = parent;  // advance the loop up the frame tree
    }

The while-loop is controlling the null 'target' and there is no place for a
release of control such that it can be changed to null.

I'll continue trying to repro this - if somebody else could sanity-check I'd
appreciate it.

shrirang khanzode

Reporter

Comment 6

•

24 years ago

still happens on win 0515 trunk. 

Steps:

type a word in composer, make it BOLD, hit ENTER
Add a default 2x2 table press OK
 click below table and write a word, make it BOLD, hit ENTER
Again try to add a 2x2 table and observe that clicking OK to add the table 
crashes the browser

Assignee

Comment 7

•

24 years ago

Er, I get the crash now (what was happening before?).

The null is not the target, btw, it is the vtable of the target.

kinmoz

Comment 8

•

24 years ago

Right target is *not* null ... but the frame it was pointing to was free'd. Look 
at the stack trace I posted above. It shows how it gets free'd ... then as the 
stack unwinds back down, some of the code tries to access the target pointer 
which is now pointing at free'd memory.

Assignee

Comment 9

•

24 years ago

So, it looks like the target frame is reflowed correctly, then we delete the
next-in-flow which ends up deleting the target frame that we just reflowed. This
seems very similar to another bug 73170 where styled frames are not getting
removed completly and end up causing grief, however here it is more immediate
because the target of a reflow is pretty quickly referenced again.

One common thread between them is the use of styling tags (<b>, <i>), but I
cannot yet figure out the content model being created because when I switch to
HTML view it is changed (and it no longer crashes). I know that we have some
nasty that takes care of blocks-in-inlines and I suspect something in there.
I'll set some breakpoints in that code and see if it is getting hit...

Assignee

Comment 10

•

24 years ago

Attached patch Patch that fixes the bug — Details — Splinter Review

Assignee

Comment 11

•

24 years ago

Patch 34919 was just attached. It fixes the crash by detecting when the 
next-in-flow of a reflowed inline frame is also the target of the current reflow 
command. If it is, we skip the deletion of the next-in-flow. This skipping of 
the deletion is already happening for frames that are BREAK_BEFORE, but I'm not 
sure what that is all about (I suppose it is possible that this is really the 
same condition but the state bit is not set for some reason). 

There appear to be no ill-effects of skipping the deletion of the Next-In-Flow 
here, and I have not been able to trigger the condition outside of Composer. 
Nonetheless, I'd like to get some more knowledgable eyes on this patch to help 
me understand if it is just semi-random hackery, or a real solution. CC'ing 
waterson for a sanity check on the patch...

Comment 12

•

24 years ago

I have to confess that this _does_ feel a little bit hacky, but I haven't
carefully read the code around here to understand why line layout thinks it can
delete the frame, and what the effect of leaving it around might otherwise be.
I'll try to take a deeper look today.

Assignee

Comment 13

•

24 years ago

It looks to me like the next in flow is always deleted when the frame is 
complete 'NS_FRAME_IS_COMPLETE(aReflowStatus)' unless there is a BREAK_BEFORE, 
in which case it pushes the frame. I think that this means that the line broke 
before the frame was placed so it wants to try and reflow it on the next line. 

But, why are we deleting the next of flow when the frame is complete? I don't 
understand that mechanism.

Anyway, the more I look at it the more this change I made appears to be just 
masking some other problem. It only catches the problem of the reflow target 
being the next in flow, but it could (conceptually) be the next in flow of the 
next in flow... Indeed, if the target of the reflow is deleted for ANY reason we 
will have the same problem. It fixes the crash, but should probably be viewed as 
a temporary fix, and suitably commented as such (unless of course a better, 
*real* solution is found - but I'm running out of time). If anybody has any 
pointers or other ideas, speak up, please.

Assignee

Comment 14

•

24 years ago

Time has run out and I have not found or heard anything better. Requesting a
review for the attached patch, even if it is a hack. What I'd like to do is get
the patch in for the 0.9.1 cut, and open another bug to find a more general
solution, or figure out why this is happening. Basically, I want to get rid of
the crash since it is so easy to cause, and then determine why the special-case
check is needed here.

I'll add more profuse comments about the temporary nature of the patch too.

Anybody willing to put their name on the r= line?

Comment 15

•

24 years ago

I cannot reproduce the problem with today's build.

Assignee

Comment 16

•

24 years ago

Still crashes for me, using a NS6 build (2001052104) and my CVS build from today.

Here are the steps I take:

0) Open new Composer window
1) type 'TEST'
2) highlight the text entered in [1] and press the B button to make it bold
3) press the right-arrow to get the cursor past the text
4) press ENTER
5) insert a table (press the table icon in the toolbar, select OK to dialog)
6) position the cursor under the new table using the mouse
7) type 'TEST2' - it should already be bold
8) press ENTER
9) insert an HR by pressing the toolbar button for HR (or you can insert another
table as in step [5]).

Comment 17

•

24 years ago

Okay, so I'm going to go out on a limb here and say that an incremental reflow 
should not be targeted at a continuing frame. Incremental reflow means ``your 
content has changed'', and for a continuing frame to be notified at all seems 
inappropriate.

That said, Peter Linss made this all happen way back in September 1999, so if 
it's really a design problem, it has been with us for a while. (Longer than 
this bug has, AFAICT.)

Comment 18

•

24 years ago

Attached patch impact >= reflow only affects first-in-flow — Details — Splinter Review

Comment 19

•

24 years ago

So, above is how I'd fix the bug (given my current amateur understanding of 
layout). Specifically, if the impact of the change is going to be 
NS_STYLE_HINT_REFLOW (or worse), then don't bother marking any of the next-in-
flows, because the reflow (or frame re-creation) will take care of it by 
definition.

If I understand correctly, there should never be a case where a continuing 
frame's style context differs from the primary frame's style context in a way 
that would necessitate reflow to the continuing frame but not the primary 
frame. (Would the style contexts ever differ _at all_?)

Feel free to thrash me. (I can here peterl's booming voice from the mountain: 
``Please refrain from criticizing a system that you do not understand1'')

Comment 20

•

24 years ago

Attached patch more conservative change; adds assertion to verify assumption — Details — Splinter Review

Comment 21

•

24 years ago

N.B. that the above patch also appears to fix bug 81860. (Marc's might too, I 
haven't tried it.)

Comment 22

•

24 years ago

cc'ing dbaron and rbs, who might be able to back me up, or debunk me.

Assignee

Comment 23

•

24 years ago

Chris, I like your change lots better. It has a wider impact than mine, but it 
sure seems like the correct approach. Continuation frames have the same content 
as their prev-in-flow, and the same pseudoclasses, so they should have the same 
style data. The only aspect of it that I do not like is that the knowledge of 
what is going to happen in the frames is spread around a bit, like now the 
FrameManager has to know that some kinds of reflow will take care of the 
continuations and some will not. I wonder if it might be better to always 
have the non-continuation take care of the continuation, so the FrameManager 
never has to? But I have to admit, it is a much better solution than my hack - 
it gets at the root of the problem instead of bandaging the symptom.

Comment 24

•

24 years ago

attinasi wrote:
>But, why are we deleting the next of flow when the frame is complete? I don't 
>understand that mechanism.

It seems this is needed if the frame is changing from a previous non-complete
status to a complete status. E.g., if a long hyphenated word is wrapped, and the
the first part is deleted (e.g., with JS), then the frame can now become
complete, and so its dangling next-in-flow needs to be cleared.

waterson's patch looks sensible to me, though marc raised some good points about
the assumptions that are being introduced in this way in the system. But the
scope seems well-defined and appears as a lesser evil than this easy to get
crash... I could only think of a case where continuations can have different
style data from the primary frame. It is the case of :first-letter. If
waterson't patch doesn't break this, then I guess things could be fine for the
time being.

Comment 25

•

24 years ago

*** Bug 81860 has been marked as a duplicate of this bug. ***

Comment 26

•

24 years ago

Marc, I think I agree that forcing the frame manager to do _anything_ with
continuing frames is wrong. If you look at the code in nsCSSFrameConstructor
(e.g., ProcessRestyledFrames(), AttributeChanged() et al.), it looks like the
only case where we take advantage of the fact that we're walking continuations
is when the change is NS_STYLE_HINT_VISUAL. Specifically, in this case, we avoid
making the frame invalidate its continuations (but that seems silly, and more
like something nsSplittableFrame could do on its own).

The only other quesetion I have is, ``why did all these bugs crop up all of a
sudden''? Were they latent? Are we (well, I'll speak for myself -- am I) making
an invalid assumption about incremental reflow?

Anyway, I guess I'd say that in some sense, my patch is as much of a band-aid as
yours, and the real fix would be to refactor this logic so that frame manager
only worried about primary frames, and let the primary frame handle its own
response to the style change.

Comment 27

•

24 years ago

That might be a performance boost as well since that will cut down on the number
of re-resolve done for continuations... And speaking of this, the incertitude so 
far with your patch lies there: that some continuations that are left in the 
flow may wrongly retain the style context with which they were init'ed. 
Re-factoring the logic will help to clear this doubt for the long run.

I tend to agree that these were latent bugs (as we have seen, incremental reflow
isn't as optimized as one would wish). If the whole lot is reflowed, chances are 
that there won't be any bugs at all. The bugs are being exposed by the ongoing 
push to make incremental reflow what it really should be. (These Ctrl+- zoom 
with _immediate access_ to vulnerable wrapped links, and buster's 
IsIncrementalDamageConstrained() -- according to "Additional Comments From 
kin@netscape.com 2000-08-16 14:32" on bug 45152", are relatively recent 
additions.)

Assignee

Comment 28

•

24 years ago

Why don't we get this fix in and work on the encapsulation of the continuing
frame stuff later? I would like to make this crash go away for 0.9.1 and we are
running out of time. sr=attinasi on waterson's patch (id=35542)

Assignee

Comment 29

•

24 years ago

BTW: this was not crashing on Netscape 6.01 - I'm not sure if the editorRules
have changed, or if something else has changed, but I'm not as convinced as rbs
that this is simply latent bad code that is being flushed out due to recent
incremental reflow changes. In the case of the editor, there is no incremental
reflow happening here, as these content insertions and deletions are pretty much
syncronous. I'd like to try some older builds and see where this started, but
that takes so much time... but it coul dhelp us understand why these started
cropping up.

Comment 30

•

24 years ago

So 6.01 doesn't have the bug, and I couldn't reproduce its dupplicate bug 81860 
(Ctrl+-) in m0.9. I haven't tried to reproduce the present bug in m0.9 but 
according to its date, it was entered soon after the branch cut for m0.9. And 
bug 81860 only happens inside a table-cell.

Time is running out though, and to join marc in his pragmatism, r=rbs on the 
band-aid hack that will stop the crash in m0.9.1.

Comment 31

•

24 years ago

Looking again at your patch, rather than breaking out of the loop, you could
instead keep going, but just add the primary frame to the change list. This way, 
everybody will get an updated style context, but only the primary frame will 
kick off the reflow. I will feel more secure with a patch along these lines, as 
it will guarantee that all style contexts will be kept up-to-date in all 
situations. And if the primary frame later wants to reflow its continuations 
(i.e., not delete them), unexpected side-effects will be reduced.

Comment 32

•

24 years ago

rbs, good point re: style contexts; however, the bad news is that stuff gets
added to the change list way down in the bowels of ReResolveStyleContext().

I was about to write that I've gone through the HTML frames to verify that
everyone is, in fact, calling through to nsFrame::Init() which will share the
style context from the first-in-flow, and then not doing any monkey-business
with the style context afterwards. (The one exception I found was the button
frame, but that would never be continued.)

I'll attach a naive test case which I think illustrates that right stuff is
happening during a restyle; can you think of a better way to stress this?

Comment 33

•

24 years ago

Attached file test visual, reflow, reframe, and first-letter under restyle — Details

Comment 34

•

24 years ago

The thing is, I am behind a firewall at the momemnt, and I don't have a Mozilla 
build and cannot try things out to see for myself :-)

What happens when you add this case to your little test suite:

body.styled p.firstletter:first-letter { color: blue; }

Comment 35

•

24 years ago

And ...

<style>
[...]
p.floated { float: left; }
</style>

[...]
<p class="floated firstletter">...</p>

Comment 36

•

24 years ago

I may be missing the target with my rules. Want I aiming at are some rules that
will select the -non- first-letter portion. That portion is a continuation with 
its own distinct style context.

Comment 37

•

24 years ago

Unfortunately, it looks like the attribute change on the body doesn't trigger
re-resolution of the :first-line or :first-letter pseudos, so I guess we're
``safe'' for now. :-/

I've filed bug 82289.

Comment 38

•

24 years ago

So so. Not much to add then. Go ahead with your check in.