Summary: | REGRESSION: Safari crashes when printing a google map w/directions | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Trey Matteson <trey> | ||||||||
Component: | Printing | Assignee: | Trey Matteson <trey> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Critical | CC: | trey | ||||||||
Priority: | P2 | ||||||||||
Version: | 420+ | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.4 | ||||||||||
URL: | http://maps.google.com/maps?f=d&hl=en&saddr=94025&daddr=10+market+st,+san+francisco | ||||||||||
Attachments: |
|
Description
Trey Matteson
2006-03-23 22:17:53 PST
Created attachment 7267 [details]
partial reduction
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. 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. 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 ? Created attachment 7269 [details]
proposed patch
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. 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 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 on attachment 7269 [details]
proposed patch
r=me
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.)
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 on attachment 7290 [details]
patch incorporating review comments
r=me
Landed back on 2006-04-04. |