Bug 23564 - REGRESSION (r39230-39286): crash loading page that changes <input> display type and then calls innerHTML
Summary: REGRESSION (r39230-39286): crash loading page that changes <input> display ty...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.5
: P1 Normal
Assignee: Darin Adler
URL:
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2009-01-26 20:56 PST by Douglas Yung
Modified: 2009-03-11 16:45 PDT (History)
2 users (show)

See Also:


Attachments
Failure callstack (31.51 KB, text/plain)
2009-01-26 21:06 PST, Douglas Yung
no flags Details
HTML page being rendered (44.13 KB, text/html)
2009-01-26 21:07 PST, Douglas Yung
no flags Details
Crash stack trace with no plug-ins (25.95 KB, text/plain)
2009-01-29 01:33 PST, Douglas Yung
no flags Details
Failure stack trace with debug build of 40359 (29.51 KB, text/plain)
2009-01-29 04:20 PST, Douglas Yung
no flags Details
patch (4.44 KB, patch)
2009-01-29 15:34 PST, Justin Garcia
no flags Details | Formatted Diff | Diff
patch (3.98 KB, patch)
2009-03-10 06:32 PDT, Darin Adler
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Douglas Yung 2009-01-26 20:56:57 PST
This seems to be a recent regression within the past few days since I'm pretty sure this was working, and it does not repro in the released official Safari build.  I use a paid site www.expertflyer.com, and when clicking on a link from inside their site, I can reliable get a crash in WebCore::HTMLElement::isContentEditable().  I will attach the full callstack that I get to this bug as well as the HTML page that it was trying to render at the time of the crash.  The page loads fine by itself, but the link which causes the crash calls tries to re-render the webpage but with certain values pre-filled in which seems to be causing the crash.
Comment 1 Douglas Yung 2009-01-26 21:06:27 PST
Created attachment 27065 [details]
Failure callstack
Comment 2 Douglas Yung 2009-01-26 21:07:43 PST
Created attachment 27066 [details]
HTML page being rendered

The page renders fine normally, but when invoked from another page that tries to pre-populate 3 fields, it causes a crash
Comment 3 Alexey Proskuryakov 2009-01-28 02:44:41 PST
Could you please use nightly builds to isolate when exactly the regression happened? Without this, it would be very hard to fix this, given that we have no way to reproduce the crash.
Comment 4 Douglas Yung 2009-01-28 14:27:52 PST
It appears the regression was around longer than I thought. Doing a binary search, I was able to repro the failure back to build 39286.  The nightly build of version 39230 does not have this problem. Hope this helps
Comment 5 Alexey Proskuryakov 2009-01-28 14:56:57 PST
<rdar://problem/6537238>
Comment 6 Deirdre Saoirse Moen 2009-01-28 15:59:30 PST
Are you able to reproduce without the unsupported third-party add-ons installed? (e.g., /Library/InputManagers/Inquisitor/Inquisitor.bundle/)
Comment 7 Douglas Yung 2009-01-28 17:28:18 PST
(In reply to comment #6)
> Are you able to reproduce without the unsupported third-party add-ons
> installed? (e.g., /Library/InputManagers/Inquisitor/Inquisitor.bundle/)
> 

Yes, the crash still repros on 40306 with Inquisitor uninstalled.
Comment 8 Alexey Proskuryakov 2009-01-28 22:34:18 PST
Oops, I should have asked that from the start. Could you please also try removing the following, and upload a new crash log if the problem persists?

  0x542000 -   0x555fff +org.andymatuschak.Sparkle 1.5 Beta (bzr) (337) <d57260aef46cb3000f771d53364e40c8> /Applications/WebKit.app/Contents/Frameworks/10.5/Sparkle.framework/Versions/A/Sparkle
  0x580000 -   0x585fff  com.apple.FolderActionsMenu 1.3.2 (1.3.2)   0x5e8000 -   0x5e9fff +com.1passwd.InputManager 2.5.12 (6253) <fbd31459857fa286d04479765637587d> /Library/InputManagers/1PasswdIM/1PasswdIM.bundle/Contents/MacOS/1PasswdIM
0x134c5000 - 0x1354cfef +com.onepasswd.onepasswdsafaribundle 2.9.8.BETA-2 (7330) <52e6263e0cba3ca7c205dc790b858b05> /Applications/1Password.app/Contents/Extensions/OnePasswdSafari.bundle/Contents/MacOS/OnePasswdSafari
0x18752000 - 0x18753ff7 +com.google.GoogleNotifierQuickAddCMPlugin 1.9.100 (1.9.100.215) /Users/dyung/Library/Contextual Menu Items/Google Notifier Quick Add CM Plugin.plugin/Contents/MacOS/Google Notifier Quick Add CM Plugin
0x18758000 - 0x1875affe  com.apple.AutomatorCMM 1.1 (160) 0x1aa3e000 - 0x1aa3ffff +com.vmware.FusionVMDKPlugIn ??? (0.0.1d2) /Library/Contextual Menu Items/FusionVMDKPlugIn.plugin/Contents/MacOS/FusionVMDKPlugIn

1Passwd is particularly suspicious.
Comment 9 Douglas Yung 2009-01-29 01:33:06 PST
Created attachment 27141 [details]
Crash stack trace with no plug-ins

This is a stack trace of the failure with build #40351 on another machine of mine running 10.5.6 PPC since I didn't want to mess up my main machine. I removed as many plug-ins as I could (sparkle seems to come with Webkit and prevents webkit from loading if I remove it). Comparing the this with the previous stack trace, they are slightly different, but they were obtained with the same repro steps.
Comment 10 Alexey Proskuryakov 2009-01-29 01:54:56 PST
Thank you for the confirmation!
Comment 11 Douglas Yung 2009-01-29 04:20:10 PST
Created attachment 27146 [details]
Failure stack trace with debug build of 40359

I was curious, so I pulled down the webkit sources (changelist 40539), and did debug build. The attached is the stack trace at the crash I got when running the debug bits I built.
Comment 12 Justin Garcia 2009-01-29 14:26:03 PST
Not sure exactly what's causing the crash but we could avoid all the code above createMarkup by being a little smarter about when we call DeleteButtonController::disable() and enable().  We don't need to call it here because there is no deletion UI to enable/disable.
Comment 13 Justin Garcia 2009-01-29 15:34:44 PST
Created attachment 27166 [details]
patch
Comment 14 Justin Garcia 2009-01-29 15:36:22 PST
Comment on attachment 27166 [details]
patch

I probably shouldn't fix it in this way, it's a recent regression and we should try to get to the bottom of it instead of just working around it.
Comment 15 Darin Adler 2009-01-29 16:11:57 PST
How about making it more robust this way, and *also* separately getting to the bottom of it?
Comment 16 Alexey Proskuryakov 2009-01-29 23:45:17 PST
Comment on attachment 27166 [details]
patch

+    // This avoids a crash (<rdar://problem/6537238>).

I know this patch isn't up for review any more, but I think that giving a Bugzilla link would be more helpful.
Comment 17 Darin Adler 2009-03-10 06:31:40 PDT
Justin’s patch here is still a good idea, but it’s not needed to fix the bug.
Comment 18 Darin Adler 2009-03-10 06:32:40 PDT
Created attachment 28435 [details]
patch
Comment 19 Alexey Proskuryakov 2009-03-10 07:04:15 PDT
Is it the best moment to update layout after disabling delete button and before appendMarkup()? The crash  happens when enabling the delete button, so I wonder if disabling it (if actually visible) could be problematic.

I guess I just still don't understand what the root cause is.
Comment 20 Darin Adler 2009-03-10 07:39:52 PDT
(In reply to comment #19)
> Is it the best moment to update layout after disabling delete button and before
> appendMarkup()?

In the case where disabling the delete button actually does DOM modifications, we need to update layout after that. But really, I was just matching the structure of the other createMarkup function.

> The crash happens when enabling the delete button, so I wonder
> if disabling it (if actually visible) could be problematic.

Actually, the crash happens inside code that's trying to decide whether to add the delete button inside enable(). It's going to decide not to add it, but in the process it's going to call updateRendering at "just the wrong time".

The reason this change fixes the bug is that it makes sure that renderers are already updated before that point. The updateRendering call ends up being a no-op.

> I guess I just still don't understand what the root cause is.

The createMarkup functions both use APIs such as isBlock that require that layout and rendering be up to date, so accordingly they need to call updateLayout -- otherwise they could run into nodes without renderers or nodes where the renderers don't reflect the latest DOM changes.

Once we fix the code so that's true, there is no longer any crash in this case.

There are multiple other things that could be improved here:

    - It's strange that the DOM isContentEditable functions do the updateRendering calls inside them. That's not generally our design and it would be best if we came up with a better system to make sure those functions are only called when renderers are up to date.

    - It's suboptimal that the delete button controller has to decide all over again whether it needs to be enabled when we already know it wasn't needed. createMarkup really wants to set it aside and then bring it back, and ideally should do less work when it's not involved at all.

    - When you change the style of an <input> element sufficiently that it blows away and recreates the shadow DOM, you lose the selection. This is a separate bug that should be fixed.

The only reason that updateRendering can make an HTML element get deleted is that the element is inside a shadow DOM tree inside a renderer that gets destroyed and recreated by the updateRendering process. Normally updating rendering wouldn't do anything arbitrary to the DOM.
Comment 21 Darin Adler 2009-03-10 07:40:43 PDT
Mitz helped me analyze this yesterday, which is why he understood and was able to review+ the patch, by the way.
Comment 22 Darin Adler 2009-03-10 07:47:13 PDT
http://trac.webkit.org/changeset/41552
Comment 23 Darin Adler 2009-03-10 12:42:24 PDT
This fix has an unfortunate side effect of making innerHTML force layout. That is going to be unwanted and slow. I am revising the fix now.
Comment 24 Darin Fisher (:fishd, Google) 2009-03-11 16:45:33 PDT
Perf regression tracked in bug 24517.