Bug 7951 - REGRESSION: Safari crashes when printing a google map w/directions
Summary: REGRESSION: Safari crashes when printing a google map w/directions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Printing (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Critical
Assignee: Trey Matteson
URL: http://maps.google.com/maps?f=d&hl=en...
Keywords:
Depends on:
Blocks:
 
Reported: 2006-03-23 22:17 PST by Trey Matteson
Modified: 2006-04-09 16:46 PDT (History)
1 user (show)

See Also:


Attachments
partial reduction (25.85 KB, text/html)
2006-03-23 22:22 PST, Trey Matteson
no flags Details
proposed patch (2.85 KB, patch)
2006-03-23 23:48 PST, Trey Matteson
hyatt: review-
Details | Formatted Diff | Diff
patch incorporating review comments (2.57 KB, patch)
2006-03-24 17:09 PST, Trey Matteson
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Trey Matteson 2006-03-23 22:17:53 PST
Go to the URL above.  Print.  Hit Preview.  Crash with the backtrace below.

The bug is a little finnicky to repro because it relies on reuse of some freed memory.  On a different version of the app/OS, you may need to try a different source/destination for the directions, or try printing a few times to get it to croak.

#0  0x01b29f70 in WebCore::DataRef<WebCore::StyleSurroundData>::operator-> (this=0x1c) at /Volumes/Whopper/WebKit/WebCore/rendering/DataRef.h:54
#1  0x01b2a08c in WebCore::RenderStyle::borderRight (this=0x0) at /Volumes/Whopper/WebKit/WebCore/rendering/render_style.h:1132
#2  0x019e29bc in WebCore::RenderTableCell::collapsedLeftBorder (this=0x122dd55c, rtl=false) at /Volumes/Whopper/WebKit/WebCore/rendering/RenderTableCell.cpp:238
#3  0x019e3d6c in WebCore::RenderTableCell::borderLeft (this=0x122dd55c) at /Volumes/Whopper/WebKit/WebCore/rendering/RenderTableCell.cpp:478
#4  0x019626f8 in WebCore::RenderBlock::calcMinMaxWidth (this=0x122dd55c) at /Volumes/Whopper/WebKit/WebCore/rendering/RenderBlock.cpp:2709
#5  0x019e5838 in WebCore::RenderTableCell::calcMinMaxWidth (this=0x122dd55c) at /Volumes/Whopper/WebKit/WebCore/rendering/RenderTableCell.cpp:88
#6  0x019ba488 in WebCore::RenderObject::recalcMinMaxWidths (this=0x122dd55c) at /Volumes/Whopper/WebKit/WebCore/rendering/RenderObject.cpp:2284
#7  0x019ba2a0 in WebCore::RenderObject::recalcMinMaxWidths (this=0x1260482c) at /Volumes/Whopper/WebKit/WebCore/rendering/RenderObject.cpp:2269
#8  0x019ba2a0 in WebCore::RenderObject::recalcMinMaxWidths (this=0x1260315c) at /Volumes/Whopper/WebKit/WebCore/rendering/RenderObject.cpp:2269
#9  0x019ba2a0 in WebCore::RenderObject::recalcMinMaxWidths (this=0x122fa5ac) at /Volumes/Whopper/WebKit/WebCore/rendering/RenderObject.cpp:2269
#10 0x019e7f0c in WebCore::RenderTable::recalcMinMaxWidths (this=0x122fa5ac) at /Volumes/Whopper/WebKit/WebCore/rendering/RenderTable.cpp:552
#11 0x019ba2a0 in WebCore::RenderObject::recalcMinMaxWidths (this=0x122aa49c) at /Volumes/Whopper/WebKit/WebCore/rendering/RenderObject.cpp:2269
#12 0x019ba2a0 in WebCore::RenderObject::recalcMinMaxWidths (this=0x124b1eec) at /Volumes/Whopper/WebKit/WebCore/rendering/RenderObject.cpp:2269
#13 0x019ba2a0 in WebCore::RenderObject::recalcMinMaxWidths (this=0x1249ec5c) at /Volumes/Whopper/WebKit/WebCore/rendering/RenderObject.cpp:2269
#14 0x019ba2a0 in WebCore::RenderObject::recalcMinMaxWidths (this=0x122a208c) at /Volumes/Whopper/WebKit/WebCore/rendering/RenderObject.cpp:2269
#15 0x019ba2a0 in WebCore::RenderObject::recalcMinMaxWidths (this=0x106e32bc) at /Volumes/Whopper/WebKit/WebCore/rendering/RenderObject.cpp:2269
#16 0x019ba2a0 in WebCore::RenderObject::recalcMinMaxWidths (this=0x1247474c) at /Volumes/Whopper/WebKit/WebCore/rendering/RenderObject.cpp:2269
#17 0x0197b99c in WebCore::RenderCanvas::layout (this=0x1247474c) at /Volumes/Whopper/WebKit/WebCore/rendering/RenderCanvas.cpp:112
#18 0x0189fff4 in WebCore::FrameView::layout (this=0xd46da10) at /Volumes/Whopper/WebKit/WebCore/page/FrameView.cpp:415
#19 0x01879084 in WebCore::Frame::forceLayout (this=<incomplete type>) at /Volumes/Whopper/WebKit/WebCore/page/Frame.cpp:2925
#20 0x018c2f78 in -[WebCoreFrameBridge forceLayoutAdjustingViewSize:] (self=<incomplete type>, _cmd=0x90a1380c, flag=1 '\001') at /Volumes/Whopper/WebKit/WebCore/kwq/DeprecatedPtrList.h:891
#21 0x0037a5f0 in -[WebHTMLView layoutToMinimumPageWidth:maximumPageWidth:adjustingViewSize:] (self=0x122ae230, _cmd=0x90a1f200, minPageWidth=0, maxPageWidth=0, adjustViewSize=1 '\001') at /Volumes/Whopper/WebKit/WebKit/WebView/WebHTMLView.m:2306
#22 0x0037f1c8 in -[WebHTMLView _setPrinting:minimumPageWidth:maximumPageWidth:adjustViewSize:] (self=0x122ae230, _cmd=0x909f87f4, printing=0 '\0', minPageWidth=0, maxPageWidth=0, adjustViewSize=1 '\001') at /Volumes/Whopper/WebKit/WebKit/WebView/WebHTMLView.m:3060
#23 0x0037f774 in -[WebHTMLView _endPrintMode] (self=0x122ae230, _cmd=0x414a84) at /Volumes/Whopper/WebKit/WebKit/WebView/WebHTMLView.m:3128
#24 0x003801e0 in -[WebHTMLView endDocument] (self=0x122ae230, _cmd=0x90a115d0) at /Volumes/Whopper/WebKit/WebKit/WebView/WebHTMLView.m:3241
Comment 1 Trey Matteson 2006-03-23 22:22:04 PST
Created attachment 7267 [details]
partial reduction
Comment 2 Trey Matteson 2006-03-23 22:24:33 PST
I created an ugly "reduction" for this bug, which amounted to some de-obfuscation of the page, commenting out certain parts, and NOP'ing some elements by adding "xx" to their names.  The main goal was to reduce the number of table cells, since they were at the heart of the bug.  The ugly techniques stemmed from the fact that when I actually removed code the bug would change places, presumably because the heap access sequence would change.
Comment 3 Trey Matteson 2006-03-23 22:33:52 PST
I believe this is a regression from stock 10.4.5 (at least I can't make it crash).  It would be interesting to hear from an expert in the code if there is any idea why this would have regressed.
Comment 4 Chris Petersen 2006-03-23 22:53:24 PST
I can't reproduce this issue under 10.4.5 with WebKit-SVN-r13461. 

Trey: Does this still happen for you with the latest Nightly build ?
Comment 5 Trey Matteson 2006-03-23 23:48:15 PST
Created attachment 7269 [details]
proposed patch
Comment 6 Trey Matteson 2006-03-23 23:51:37 PST
Hmm, no, it doesn't happen for me on TOT.  On the other hand, I believe I found a clear logic error in the code, whose effects I observed in gdb, and which when fixed did definitely make the problem go away (all that before I updated to TOT).  I will consult the table experts as to whether another related fix went in recently, and whether my change makes sense.

The bug was so sensitive to the conditions of heap usage that it's quite possible that the change I got when I updated masked it.
Comment 7 Trey Matteson 2006-03-24 10:59:38 PST
More info on why this now works on TOT:  I tried running with MallocScribble, and still did not see a crash.  I then debugged, and found a big difference from the code path I was previously seeing.  During the relayouts during printing, we no longer do a full decent of recalcMinMaxWidths() through the render tree.  The topmost block has m_recalcMinMax false, so we never get into the recursion that was previously running into the freed table cells.

I believe my proposed change is still an improvement in safety, but I don't know the conditions now needed to force this issue to cause a crash.
Comment 8 Dave Hyatt 2006-03-24 15:47:25 PST
Comment on attachment 7269 [details]
proposed patch

I'd like to understand this problem a bit better.  recalcMinMaxWidths is supposed to just be the generic non-virtual workhorse that calls the virtual calcMinMaxWidth methods.

Could the cells not force a section recalc when needed?  

It's also not clear to me what causes this problem, since I thought the recalc was done when cells got added/removed.  Is printing causing a change in the cell composition of the table?
Comment 9 Dave Hyatt 2006-03-24 15:53:38 PST
Comment on attachment 7269 [details]
proposed patch

r=me
Comment 10 Dave Hyatt 2006-03-24 15:56:42 PST
Comment on attachment 7269 [details]
proposed patch

Sorry to flip-flop.  I do think doing this in the cell is slightly better because

(a) You avoid making recalcMinMaxWidths virtual

and

(b) Your current patch does the section recalc even when the table may not need to recompute its min/max width (or cells may not etc.)
Comment 11 Trey Matteson 2006-03-24 17:09:07 PST
Created attachment 7290 [details]
patch incorporating review comments

Same idea, this time with the cells requesting that the sections be made up to date.
Comment 12 Dave Hyatt 2006-04-03 15:02:37 PDT
Comment on attachment 7290 [details]
patch incorporating review comments

r=me
Comment 13 Darin Adler 2006-04-09 16:46:11 PDT
Landed back on 2006-04-04.