Closed Bug 113235 Opened 23 years ago Closed 22 years ago

hang loading page; font element wrapped around malformed tables

Categories

(Core :: Layout: Tables, defect, P2)

Product:
Core
Component:
Layout: Tables
Platform:
All
Windows 2000
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: chris, Assigned: karnaze)

References

(
URL
)

Details

(Keywords: hang, Whiteboard: [awd:tbl][p2][adt2 RTM])

Attachments

(5 files, 2 obsolete files)

worst case;
46.81 KB, text/html
Details
characteristic stack trace; ignore top of stack
9.69 KB, text/plain
Details
simple test case where nsCSSFrameConstructor::ReframeContainingBlock becomes an issue
567 bytes, text/html
Details
more test cases for the patch
4.01 KB, text/html
Details
revised patch with reviewers' suggestions
10.22 KB, patch
karnaze
: review+
karnaze
: superreview+
jesup
: approval+
Details | Diff | Splinter Review 
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.6) Gecko/20011120
BuildID:    2001112009

Note: I've also tried this on Linux (Debian Testing/Unstable) Mozilla 0.9.6:
Mozilla/5.0 (X11;U;Linux i686;en-US;rv:0.9.6) Gecko/20011201 from Debian package
2:0.9.6-6, and I get the same behaviour.

When going to view the above URL Mozilla appears to hang. Task Manger (and/or
top) shows that Mozilla is using 100% CPU time, and the page is only half
displayed. The only option seems to be to kill the processes.

Reproducible: Always
Steps to Reproduce:
1. Visit above url (!)


Actual Results:  100% CPU time showing on task management applications. Display
not updating (not updating after removing overlapping windows)

Expected Results:  Rendered the page - Page works in IE6 and NS4.7. Page is
Table of contents to IBM DB2 SQL Manual
confirming with build 2001120208 on win2k.
page also works fine in opera6
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: hang, top100
confirming build 2001120223 on Linux
think this is related to bug 74888.
Severity: normal → major
Hardware: PC → All
Keywords: 4xp
Keywords: perf
QA Contact: sairuh → jrgm
-> HTML Tables, and it's just a plain old bug

There is some malformed <table>/<tr>/<td> in that page. In the simplest case, 
this winds up creating duplicate content in the document. In the worst case, 
like this page, where the text is a large collection of inline elements, it 
never returns.

<html>
<body>
<table border>
  <tr valign="top">
    <td>x</td>
    <td>
      <!-- this font, and the malformed table below trigger the bug -->
      <font face="Arial, sans-serif" size="-1">
      <table border>
        <tr>
	  <td width="10">&nbsp;</td>
	  <td width="443" valign="top" align="left">
	<tr>
          <td>Search again...</td>  
        </tr>
	<table>
	  <tr>
            This text node only exists once in the original document!
            If it were a large collection of inlines, the page would 
            hang forever.
          </tr>
          <td>x</td>
          <td>x</td>
          </tr>
        </table>
          </td>
        </tr>
      </table>
    </td>
  </tr>
</table>
</body>
</html>
Assignee: pchen → karnaze
Component: XP Apps → HTMLTables
Keywords: 4xp, perf, top100
QA Contact: jrgm → amar

    
    

    
  

      
      
      
      

        Attached file
          
        worst case; 
        — 
      

    


  
same as previous example, but add in the large set of inline elements from the
IBM page

    
  
Summary: 100% CPU on viewing page → hang loading page; font element wrapped around malformed tables

    
    

    
  
Going back in old builds, this URL loaded in trunk 8/14, but hangs in 
trunk 8/17. Bernd, possibly your checkin for bug 32841 on 8/15, if you 
wanted to have a look at this.

    
    

    
  
I dont even hit the table layout code. I loaded the worst case in the viewer and
placed a breakpoint in BasicTableLayoutStrategy.cpp::Initialize. This breakpoint
was never reached. Instead when stopping the program it shows the following
stack trace:

SHELL32! 7fcb8ac1()

And thats all.

    
    

    
  


  
That's the sort of stack that I get when hung. (Ignore the specific top stack
frames in the stack attached; the more relevant bits are the frame constructor
stuff). 

Maybe this isn't tables per se, although without a table in the IBM page, I 
don't get a hang at all when loading that content. There is often, but not
always a 'nsCSSFrameConstructor::ConstructTableCellFrame' frame on the stacks.

I dunno. What happens when a bare <tr> or bare <td> is seen in content. Does 
nsCSSFrameConstructor build a frame for it independent of table layout
strategy?

    
    

    
  
The BasicTableLayoutStrategy is a part of the table reflow. To my knowledge
first the frames get constructed and then they get the InitialReflow, this is
the moment when table layout strategy comes in. So it looks like it hangs before
that. Adding Mr. FrameConstruction to CC as he can probably better explain what
is going on.

    
    

    
  
Temporarily moving to future until a milestone can be assigned. 
Status: NEW → ASSIGNED
Target Milestone: --- → Future

    
  
Whiteboard: [awd:tbl][p2]

    
    

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

    
    

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

    
    

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

    
    

    
  
Comment on attachment 60254 [details]
simple test case showing duplicate content

this test case doesn't show duplicate content anymore. But the other testcase,
and the original IBM URL will hang the app (100% CPU)

        Attachment #60254 -
        Attachment is obsolete: true

    
    

    
  
Pulling back from temporary home of Future :-). This now has four duplicates.
I think this needs a little bit of a look (and reassignment from Tables if it 
isn't really tables code that is the problem).
Severity: major → critical
Keywords: nsbeta1
Target Milestone: Future → ---

    
    

    
  


  
When the <tr>'s contents are put inside <td> </td>, ReframeContainingBlock is
not called in this test case. When more content is put inside, the number of
calls becomes an issue. Attachment #2 [details] [diff] has so much content inside the <tr>, that
I am seeing a hang because of the number of times things are getting reframed.

    
    

    
  
What is the content model that eventually gets created? Is the sink's fixup
doing ContentInserted instead of ContentAppended? (We can optimize the latter
much more easily...)

    
    

    
  
The parser rightly moves the content from inside the <tr> with no <tr> to 
outside the table. but this causes a number of calls to ContentInserted. This 
doesn't happen if the table is removed.

    
    

    
  
Should have said "<tr> with no <td>"

    
    

    
  


  
The patch avoids most if not all of the calls to ReframeContainingBlock in the
url and test. The url still takes a long time to come up, but it takes a fairly
long time in IE as well. I need to develop more test cases using javascript to
exercise the entire patch. I filed bug 138577 which deals with the parser
losing some of the content (e.g. <h2>) which it is supposed to move out of the
malformed table.

    
    

    
  


  

    
  

        Attachment #80040 -
        Flags: review+

    
    

    
  
Comment on attachment 80040 [details] [diff] [review]
patch to avoid reframing when content is inserted inside a special frame

IsInlineFrame2 needs a PRECONDITION for aFrame, and possibly a beeter name
(BTW, did you try changing the original IsInlineFrame method to use the
styleDisplay method, or was that too risky?)

Incomplete comment?
+      // if !aPrevSibling then aChild goes into the 1st inline frame which is
aParentFrame, otherwise

Incomplete comment:
+    // reframe to avoid

r=attinasi

    
    

    
  
nsbeta1-. Has fix, but the change is too large to be accepted for the branch.
Keywords: nsbeta1 → nsbeta1-

    
    

    
  
Comment on attachment 80040 [details] [diff] [review]
patch to avoid reframing when content is inserted inside a special frame

I don't like the timid style of NS_ENSURE-ing everything. Have some conviction,
man! You know what you're doing.

Couldn't we just return if !IsInlineFrame2()? No need to propagate kipp's
super-indented style here.

>+  if (IsInlineFrame2(aParentFrame)) {
>+    // find out if aChild is a block or inline
>+    PRBool childIsBlock = PR_FALSE;
>+    if (aChild->IsContentOfType(nsIContent::eELEMENT)) {
>+      nsCOMPtr<nsIStyleContext> styleContext;
>+      ResolveStyleContext(aPresContext, aParentFrame, aChild, getter_AddRefs(styleContext)); NS_ENSURE_TRUE(styleContext, PR_TRUE);

We don't check if ResolveStyleContext succeeds anywhere else. Why start now?

>+      const nsStyleDisplay* display = 
>+        (const nsStyleDisplay*) styleContext->GetStyleData(eStyleStruct_Display); NS_ENSURE_TRUE(display, PR_TRUE); 

Likewise.

>+      childIsBlock = display->IsBlockLevel();
>+    }
>+    nsIFrame* prevParent; // parent of prev sibling
>+    nsIFrame* nextParent; // parent of next sibling
>+
>+    if (childIsBlock) { 
>+      if (aPrevSibling) {
>+        aPrevSibling->GetParent(&prevParent); NS_ENSURE_TRUE(prevParent, PR_TRUE);
>+        if (IsInlineFrame2(prevParent)) { // prevParent is an inline
>+          // XXX we need to find out if prevParent is the 1st inline or the last. If it
>+          // is the 1st, then aChild and the frames after aPrevSibling within the 1st inline
>+          // need to be moved to the block(inline). If it is the last, then aChild and the
>+          // frames before aPrevSibling within the last need to be moved to the block(inline). 
>+          return PR_TRUE; // For now, bail.
>+        }
>+        else { // prevParent is a block, put aChild there
>+          aParentFrame = prevParent;
>+        }

`else' after an unconditional `return' doesn't make sense.

>+      }
>+      else {
>+        nsIFrame* nextSibling = (aIndexInContainer >= 0)
>+                                ? FindNextSibling(aPresShell, aParent2, aIndexInContainer)
>+                                : FindNextAnonymousSibling(aPresShell, mDocument, aParent1, aChild);
>+        if (nextSibling) {
>+          nextSibling->GetParent(&nextParent); NS_ENSURE_TRUE(nextParent, PR_TRUE);
>+          if (IsInlineFrame2(nextParent)) {
>+            // XXX we need to move aChild, aNextSibling and all the frames after aNextSibling within
>+            // the 1st inline to the block(inline).
>+            return PR_TRUE; // for now, bail

Same.

>+          }
>+          else {
>+            // put aChild in nextParent which is the block(inline) and leave aPrevSibling null
>+            aParentFrame = nextParent;
>+          }
>+        }
>+      }           
>+    }
>+    else { // aChild is an inline
>+      // if !aPrevSibling then aChild goes into the 1st inline frame which is aParentFrame, otherwise
>+      if (aPrevSibling) {
>+        aPrevSibling->GetParent(&prevParent); NS_ENSURE_TRUE(prevParent, PR_TRUE);

Can we ever really get into a situation where the parent is null?

>+        if (IsInlineFrame2(prevParent)) { // prevParent is an inline
>+          // aChild goes into the same inline frame as aPrevSibling
>+          aPrevSibling->GetParent(&aParentFrame); NS_ENSURE_TRUE(aParentFrame, PR_TRUE);

Same.


Fix the else-after-returns, outdent, and sr=waterson. I'll let you decide what
to do with the NS_ENSUREs. ;-)

        Attachment #80040 -
        Flags: superreview+

    
    

    
  


  

        Attachment #80040 -
        Attachment is obsolete: true

    
    

    
  
FIXED_ON_TRUNK
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Priority: -- → P2
Resolution: --- → FIXED
Whiteboard: [awd:tbl][p2] → [awd:tbl][p2][FIXED_ON_TRUNK]
Target Milestone: --- → mozilla1.0.1

    
    

    
  
nsbeta1+. 
Keywords: nsbeta1- → nsbeta1+
Whiteboard: [awd:tbl][p2][FIXED_ON_TRUNK] → [awd:tbl][p2][FIXED_ON_TRUNK][adt2]
Target Milestone: mozilla1.0.1 → mozilla1.0

    
  
Blocks: 96561

    
  

        Attachment #81763 -
        Flags: superreview+

        Attachment #81763 -
        Flags: review+

    
  
Keywords: adt1.0.0, 
          
            approval

    
    

    
  
+    const nsStyleDisplay* display = 
+      (const nsStyleDisplay*) styleContext->GetStyleData(eStyleStruct_Display);  
+    childIsBlock = display->IsBlockLevel();

Can display ever come back NULL? If so, the deref will crash. If we decide to go
in the branch, and there is a chance of NULL, probably need to handle that.
Other code seems to be doing asserts and NULL checks ok.

    
    

    
  
Display should never be null. If you want to start checking pointers which 
shouldn't be null (which I did in a previous patch) then please take it up with 
waterson.

    
    

    
  
What karnaze said. ;-)

    
    

    
  
This sounds like a good candidate to take after RC2, as we are ramping down.
adt1.0.0- [adt2 RTM]
Keywords: adt1.0.0 → adt1.0.0-
Whiteboard: [awd:tbl][p2][FIXED_ON_TRUNK][adt2] → [awd:tbl][p2][FIXED_ON_TRUNK][adt2 RTM]

    
    

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

    
    

    
  
putting back adt1.0.0 nomination for rtm.

Can someone verify this on the trunk?  What other areas need to be tested prior
to landing on the branch?
Keywords: adt1.0.0- → adt1.0.0

    
    

    
  
this works for me in a trunk build on win2k. No hang for original URL or 
testcases.

    
    

    
  
adding adt1.0.0+.  Please get drivers approval and then check into the 1.0 branch.
Keywords: adt1.0.0 → adt1.0.0+

    
    

    
  
Comment on attachment 81763 [details] [diff] [review]
revised patch with reviewers' suggestions

a=rjesup@wgate.com for branch.	Please be careful and check for
bustage/regression after checkin

        Attachment #81763 -
        Flags: approval+

    
    

    
  
fixed1.0.0
Keywords: fixed1.0.0
Whiteboard: [awd:tbl][p2][FIXED_ON_TRUNK][adt2 RTM] → [awd:tbl][p2][adt2 RTM]

    
    

    
  
 Verified on branch 20020523 build. The URl does not hang.
Keywords: verified1.0.0

    
  
No longer blocks: 96561

    
    

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

    
    

    
  
verified

Status: RESOLVED → VERIFIED

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: