Bug 23564 – REGRESSION (r39230-39286): crash loading page that changes <input> display type and then calls innerHTML

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Douglas Yung]

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.

[Douglas Yung]

Created attachment 27065 [details]
Failure callstack

[Douglas Yung]

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

[Alexey Proskuryakov]

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.

[Douglas Yung]

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

[Alexey Proskuryakov]

<rdar://problem/6537238>

[Deirdre Saoirse Moen]

Are you able to reproduce without the unsupported third-party add-ons installed? (e.g., /Library/InputManagers/Inquisitor/Inquisitor.bundle/)

[Douglas Yung]

(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.

[Alexey Proskuryakov]

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.

[Douglas Yung]

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.

[Alexey Proskuryakov]

Thank you for the confirmation!

[Douglas Yung]

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.

[Justin Garcia]

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.

[Justin Garcia]

Created attachment 27166 [details]
patch

[Justin Garcia]

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.

[Darin Adler]

How about making it more robust this way, and *also* separately getting to the bottom of it?

[Alexey Proskuryakov]

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.

[Darin Adler]

Justin’s patch here is still a good idea, but it’s not needed to fix the bug.

[Darin Adler]

Created attachment 28435 [details]
patch

[Alexey Proskuryakov]

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.

[Darin Adler]

(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.

[Darin Adler]

Mitz helped me analyze this yesterday, which is why he understood and was able to review+ the patch, by the way.

[Darin Adler]

http://trac.webkit.org/changeset/41552

[Darin Adler]

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.

[Darin Fisher (:fishd, Google)]

Perf regression tracked in bug 24517.