Bug 60712 - Regression caused by changeset 86281
Summary: Regression caused by changeset 86281
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P2 Normal
Assignee: Jia Pu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-05-12 11:14 PDT by Jia Pu
Modified: 2011-05-12 18:19 PDT (History)
4 users (show)

See Also:


Attachments
Patch (v1) (7.23 KB, patch)
2011-05-12 11:32 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Patch (v2) (15.45 KB, patch)
2011-05-12 15:34 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Patch (v3) (18.57 KB, patch)
2011-05-12 16:40 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Patch (v4) (18.57 KB, patch)
2011-05-12 17:32 PDT, Jia Pu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jia Pu 2011-05-12 11:14:01 PDT
Next typing after canceling a reversion panel would hang.
<rdar://problem/9427970>
Comment 1 Jia Pu 2011-05-12 11:32:33 PDT
Created attachment 93316 [details]
Patch (v1)
Comment 2 Jia Pu 2011-05-12 11:33:53 PDT
Verified this patch with all existing automatic and manual autocorrection tests. I probably forgot to run manual tests on the offending changeset. Otherwise I would have caught this.
Comment 3 Darin Adler 2011-05-12 11:39:31 PDT
Comment on attachment 93316 [details]
Patch (v1)

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

Iā€™m really worried about the thread-safety issues here.

> Source/WebKit/mac/ChangeLog:10
> +        Need to clear m_view pointer in handleAcceptedReplacement(), since this function can be invoked
> +        by AppKit on different thread without going through dismissInternal().

Does this code have enough locking for this to be called on a different thread!?

> Source/WebKit2/ChangeLog:10
> +        Need to clear m_view pointer in handleAcceptedReplacement(), since this function can be invoked
> +        by AppKit on different thread without going through dismissInternal().

Does this code have enough locking for this to be called on a different thread!?
Comment 4 Jessie Berlin 2011-05-12 12:19:11 PDT
Comment on attachment 93316 [details]
Patch (v1)

Thanks for the review, Darin! Committed in http://trac.webkit.org/changeset/86371
Comment 5 Jia Pu 2011-05-12 12:33:36 PDT
(In reply to comment #4)
> (From update of attachment 93316 [details])
> Thanks for the review, Darin! Committed in http://trac.webkit.org/changeset/86371

Jessie, actually Darin raised a valid concern. I'm talking to some AppKit folks to gain better understanding on the threading model. I may have another change.
Comment 6 Jessie Berlin 2011-05-12 12:40:46 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 93316 [details] [details])
> > Thanks for the review, Darin! Committed in http://trac.webkit.org/changeset/86371
> 
> Jessie, actually Darin raised a valid concern. I'm talking to some AppKit folks to gain better understanding on the threading model. I may have another change.

That was a total fail by me. It was meant for another bug, not this one. Sorry!
Comment 7 Jia Pu 2011-05-12 15:32:47 PDT
Reopen since it was closed by mistake.
Comment 8 Jia Pu 2011-05-12 15:34:39 PDT
Created attachment 93357 [details]
Patch (v2)
Comment 9 Jia Pu 2011-05-12 16:02:13 PDT
Comment on attachment 93357 [details]
Patch (v2)

Having looked at the AppKit source code, it seems all callbacks eventually are delivered on main thread. So we really don't need any locking here. I will upload another patch soon.
Comment 10 Jia Pu 2011-05-12 16:40:49 PDT
Created attachment 93365 [details]
Patch (v3)
Comment 11 Darin Adler 2011-05-12 17:27:43 PDT
Comment on attachment 93365 [details]
Patch (v3)

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

> Source/WebKit/mac/ChangeLog:14
> +        Since now we don't have distincation between dismiss() and dismissSoon(). dismissSoon() has

Typo: distinction
Comment 12 Jia Pu 2011-05-12 17:32:47 PDT
Created attachment 93374 [details]
Patch (v4)

Fixed typo.
Comment 13 WebKit Commit Bot 2011-05-12 18:19:23 PDT
Comment on attachment 93374 [details]
Patch (v4)

Clearing flags on attachment: 93374

Committed r86406: <http://trac.webkit.org/changeset/86406>
Comment 14 WebKit Commit Bot 2011-05-12 18:19:27 PDT
All reviewed patches have been landed.  Closing bug.