Bug 26667 – Assertion failure in -[WebHTMLView _handleStyleKeyEquivalent:]

• Component: HTML Editing
• Status: RESOLVED FIXED

[Jeff Johnson]

Overview Description:
There is a reproducible assertion failure in -[WebHTMLView _handleStyleKeyEquivalent:] using WebKit TOT. This is a regression from Mac OS X 10.5.7 Safari 4.0.1.

Steps to Reproduce:
1) Unzip, build, and run the attached sample Xcode project "WebViewClose.zip".
2) When the web page stops loading, do command-q to quit.

Actual Results:
Assertion failure.

Expected Results:
No assertion failure.

Build Date & Platform:
git commit 401f9b797d616c4f01bfe3f730b723494afc0ec2 Tue Jun 23 23:06:36 2009
+0000, corresponding to svn r45011.

Additional Information:
The sample Xcode project is actually the same as the one in <https://bugs.webkit.org/show_bug.cgi?id=26665>. I found this bug while testing the other bug. However, the bugs do not appear to be otherwise related.

I've attached a crash log. Also, here's the backtrace:
ASSERTION FAILED: [self _webView]
(/Users/Shared/data/source/WebKit/WebKit/mac/WebView/WebHTMLView.mm:4157 BOOL -[WebHTMLView _handleStyleKeyEquivalent:](WebHTMLView*, objc_selector*, NSEvent*))
Program received signal:  “EXC_BAD_ACCESS”.
(gdb) bt
#0  0x00339880 in -[WebHTMLView _handleStyleKeyEquivalent:] (self=0xb74c60, _cmd=0x3e5e62, event=0x1c468890) at /Users/Shared/data/source/WebKit/WebKit/mac/WebView/WebHTMLView.mm:4157
#1  0x0033e8e8 in -[WebHTMLView performKeyEquivalent:] (self=0xb74c60, _cmd=0x94891a60, event=0x1c468890) at /Users/Shared/data/source/WebKit/WebKit/mac/WebView/WebHTMLView.mm:4187
#2  0x95aae8b6 in -[NSControl _performKeyEquivalent:conditionally:] ()
#3  0x95aae782 in -[NSView performKeyEquivalent:] ()
#4  0x95aae782 in -[NSView performKeyEquivalent:] ()
#5  0x95aae782 in -[NSView performKeyEquivalent:] ()
#6  0x95aae782 in -[NSView performKeyEquivalent:] ()
#7  0x95aae782 in -[NSView performKeyEquivalent:] ()
#8  0x95aae782 in -[NSView performKeyEquivalent:] ()
#9  0x95aae4eb in -[NSWindow performKeyEquivalent:] ()
#10 0x95aae1af in -[NSApplication _handleKeyEquivalent:] ()
#11 0x959cb0fb in -[NSApplication sendEvent:] ()
#12 0x9592862f in -[NSApplication run] ()
#13 0x958f5834 in NSApplicationMain ()
#14 0x00001b50 in main (argc=1, argv=0xbffff6c8) at /Users/Shared/data/source/MiniBrowser/main.m:45

[Jeff Johnson]

Created attachment 31761 [details]
Sample Xcode project

[Jeff Johnson]

Created attachment 31762 [details]
Crash log

[Jeff Johnson]

Created attachment 86129 [details]
Patch

[Alexey Proskuryakov]

As this is a regression, it would be very useful to know when this regressed.

[Jeff Johnson]

In retrospect, I'm not sure that this was actually a regression. I don't think I realized at the time I filed the bug that assertions were not compiled into release builds. I said, "This is a regression from Mac OS X 10.5.7 Safari 4.0.1", which would have been weird to say if I had been testing a particular svn revision. I'm sure I would have mentioned the svn revision if I had a known good one.

[Darin Adler]

Comment on attachment 86129 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86129&action=review

> Source/WebKit/mac/WebView/WebHTMLView.mm:4388
> +    if (_private->closed)
> +        return [super performKeyEquivalent:event];

While it’s OK to add the _private->closed check, I’m not sure it’s the best fix. There are many other entry points that do not guard this.

I suggest instead changing the assertion.

    ASSERT(_private->closed || [self _webView]);

[Jeff Johnson]

(In reply to comment #6)
> (From update of attachment 86129 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86129&action=review
> 
> > Source/WebKit/mac/WebView/WebHTMLView.mm:4388
> > +    if (_private->closed)
> > +        return [super performKeyEquivalent:event];
> 
> While it’s OK to add the _private->closed check, I’m not sure it’s the best fix. There are many other entry points that do not guard this.
> 
> I suggest instead changing the assertion.
> 
>     ASSERT(_private->closed || [self _webView]);

I don't believe that would be a good fix, because while it trivially solves the assertion failure, it makes the method logic flawed. For example:

    if (![[[self _webView] preferences] respectStandardStyleKeyEquivalents])
        return NO;

The conditional is accidentally satisfied because [self _webView] is nil, regardless of the preference setting.

[Darin Adler]

(In reply to comment #7)
> > I suggest instead changing the assertion.
> > 
> >     ASSERT(_private->closed || [self _webView]);
> 
> I don't believe that would be a good fix, because while it trivially solves the assertion failure, it makes the method logic flawed. For example:
> 
>     if (![[[self _webView] preferences] respectStandardStyleKeyEquivalents])
>         return NO;
> 
> The conditional is accidentally satisfied because [self _webView] is nil, regardless of the preference setting.

While it may bother you to run code like that that seems to do the right thing for the wrong reason, there is no reason to add code paths to handle the closed case. The code will behave harmlessly when there is no web view and in every other respect as well. We don’t need to add additional code to get the correct behavior in this odd edge case, because everything works fine.

The only thing causing trouble is the assertion.

Adding a closed check in this one method will create the false notion that we need to add it elsewhere too, and there is no evidence to suggest that is the case.

[Jeff Johnson]

(In reply to comment #8)
> (In reply to comment #7)
> > > I suggest instead changing the assertion.
> > > 
> > >     ASSERT(_private->closed || [self _webView]);
> > 
> > I don't believe that would be a good fix, because while it trivially solves the assertion failure, it makes the method logic flawed. For example:
> > 
> >     if (![[[self _webView] preferences] respectStandardStyleKeyEquivalents])
> >         return NO;
> > 
> > The conditional is accidentally satisfied because [self _webView] is nil, regardless of the preference setting.
> 
> While it may bother you to run code like that that seems to do the right thing for the wrong reason, there is no reason to add code paths to handle the closed case. The code will behave harmlessly when there is no web view and in every other respect as well. We don’t need to add additional code to get the correct behavior in this odd edge case, because everything works fine.
> 
> The only thing causing trouble is the assertion.
> 
> Adding a closed check in this one method will create the false notion that we need to add it elsewhere too, and there is no evidence to suggest that is the case.

It does bother me, and I refuse to write incorrect, fragile code like that.

Frankly, as I was composing my patch, I did get the impression that other methods in this class probably needed a closed check as well. I suspect that the absence of checks is simply the result of lazy, careless coding.

[Darin Adler]

Comment on attachment 86129 [details]
Patch

Fix your attitude if you’d like to contribute to this project.

Careless and lazy patches will not be accepted.

[Jeff Johnson]

I have no desire to work on a project that prefers hiding bugs to fixing them. Farewell.

[Darin Adler]

Created attachment 87743 [details]
Patch

[Darin Adler]

Comment on attachment 87743 [details]
Patch

Sorry there’s no regression test; I should have included one. I think someone probably could make a test with eventSender, but I’m not sure how easy it would be.

[WebKit Commit Bot]

Comment on attachment 87743 [details]
Patch

Clearing flags on attachment: 87743

Committed r82612: <http://trac.webkit.org/changeset/82612>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/82612 might have broken Qt Linux Release
The following tests are not passing:
fast/multicol/layers-in-multicol.html