Bug 60572 - REGRESSION - Canon On-screen Manual.app crashes after a search
: REGRESSION - Canon On-screen Manual.app crashes after a search
Status: RESOLVED FIXED
: WebKit
WebKit Misc.
: 528+ (Nightly build)
: Macintosh All
: P2 Normal
Assigned To:
:
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2011-05-10 12:57 PST by
Modified: 2011-05-11 10:39 PST (History)


Attachments
Patch v1 (2.95 KB, patch)
2011-05-10 13:01 PST, Brady Eidson
alice.barraclough: review+
beidson: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-05-10 12:57:20 PST
REGRESSION - Canon On-screen Manual.app crashes after a search.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libobjc.A.dylib                   0x9a4bda53 _class_getName + 10
1   libobjc.A.dylib                   0x9a4c20e8 object_getClassName + 33
2   libobjc.A.dylib                   0x9a4ca8d7 _objc_error + 57
3   libobjc.A.dylib                   0x9a4caadd __objc_error + 45
4   libobjc.A.dylib                   0x9a4c827c _freedHandler + 53
5   com.apple.Foundation              0x9aee937f -[NSConcreteNotification dealloc] + 59
6   libobjc.A.dylib                   0x9a4d7c07 _objc_rootRelease + 47
7   libobjc.A.dylib                   0x9a4d80c6 (anonymous namespace)::AutoreleasePoolPage::pop(void*) + 404
8   com.apple.CoreFoundation          0x98c9f495 _CFAutoreleasePoolPop + 53
9   com.apple.Foundation              0x9af210fd postQueueNotifications + 1149
10  com.apple.CoreFoundation          0x98d5fe6e __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__ + 30
11  com.apple.CoreFoundation          0x98ce141d __CFRunLoopDoObservers + 413
12  com.apple.CoreFoundation          0x98ca239d __CFRunLoopRun + 1261
13  com.apple.CoreFoundation          0x98ca1a9c CFRunLoopRunSpecific + 332
14  com.apple.CoreFoundation          0x98ca1948 CFRunLoopRunInMode + 120
15  com.apple.HIToolbox               0x900fa313 RunCurrentEventLoopInMode + 318
16  com.apple.HIToolbox               0x900fa0db ReceiveNextEventCommon + 381
17  com.apple.HIToolbox               0x900f9f4a BlockUntilNextEventMatchingListInMode + 88
18  com.apple.AppKit                  0x99a4e5dc _DPSNextEvent + 678
19  com.apple.AppKit                  0x99a4de49 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 113
20  com.apple.AppKit                  0x99a0feb4 -[NSApplication run] + 897
21  com.apple.AppKit                  0x99a08004 NSApplicationMain + 1047
22  jp.co.canon.ij.easy-guide-viewer    0x000028ca _start + 216
23  jp.co.canon.ij.easy-guide-viewer    0x000027f1 start + 41

The Canon app subclasses WebView, and in their own [CustomWebView dealloc] method, they manipulates WebPreferences.  One thing they do before starting a search is disabling automatic image loading.  One thing they do in their subclasses dealloc method is reenable automatic image loading.

This crash started in r66577 when we started telling all Frames in the Page that they can start image loading, which they immediately do.  This dispatches out to the delegates and re-retains the WebView, adding it to various collections and notifications.

When one of these Notifications is later dealloc'ed after the runloop has spun, the WebView *has* actually been destroyed and free'd, and the attempt to lower its ref count crashes.

In radar as <rdar://problem/9343191>
------- Comment #1 From 2011-05-10 12:59:13 PST -------
Since the trouble originates in their subclassed dealloc method, we have no idea we're about to be dealloced.  We just assume this is a normal [WebView close] and have no way of preventing the retain badness happening at this stage.

When the preference is twiddled to allow image loading, there's absolutely no reason why we'd have to start the loads synchronously.  Putting that on a 0-delay timer makes this go away which no real side effects.
------- Comment #2 From 2011-05-10 13:01:32 PST -------
Created an attachment (id=92997) [details]
Patch v1
------- Comment #3 From 2011-05-10 13:26:29 PST -------
Fixed in r86179
------- Comment #4 From 2011-05-10 14:12:05 PST -------
I committed without the ChangeLog and in-code comment I'd meant to include.
------- Comment #5 From 2011-05-10 14:19:58 PST -------
Updated with comments in 86180
------- Comment #6 From 2011-05-10 16:10:51 PST -------
http://trac.webkit.org/changeset/86180 might have broken Windows XP Debug (Tests)
The following tests are not passing:
media/controls-without-preload.html
------- Comment #7 From 2011-05-10 16:18:57 PST -------
(In reply to comment #6)
> http://trac.webkit.org/changeset/86180 might have broken Windows XP Debug (Tests)
> The following tests are not passing:
> media/controls-without-preload.html

Appreciate the suggestion, but it's extremely unlikely that adding comments to code broke a layout test.  (And almost as unlikely that the original preferences-related patch broke a media test that doesn't invoke that preference)
------- Comment #8 From 2011-05-10 22:34:29 PST -------
> Appreciate the suggestion, but it's extremely unlikely that adding comments to code broke a layout test.  (And almost as unlikely that the original preferences-related patch broke a media test that doesn't invoke that preference)

Yeah, we really need to work on the spaminess of these notifications.  Sometimes I'm tempted to turn them off..
------- Comment #9 From 2011-05-10 22:38:07 PST -------
@abarth, @beidson:  I'll spend some time tomorrow working on making the SB smarter about which bugs it comments on.  There is a lot of low hanging fruit here.
------- Comment #10 From 2011-05-11 10:39:11 PST -------
Adam has turned off notifications when SB would notify more than 3 bugs.  I'm not sure if that would have helped here, but it should reduce the times when he's just "spamming".