Bug 60712

Summary:

Regression caused by changeset 86281

Product:

WebKit

Reporter:

Jia Pu <jiapu.mail>

Component:

HTML Editing

Assignee:

Jia Pu <jiapu.mail>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, darin, ddavidso, jberlin

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Mac (Intel)

OS:

OS X 10.6

Attachments:

Description	Flags
Patch (v1)	none
Patch (v2)	none
Patch (v3)	none
Patch (v4)	none

Jia Pu

Reported 2011-05-12 11:14:01 PDT

Next typing after canceling a reversion panel would hang. <rdar://problem/9427970>

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Jia Pu

Comment 1 2011-05-12 11:32:33 PDT

Created attachment 93316 [details] Patch (v1)

Jia Pu

Comment 2 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.

Darin Adler

Comment 3 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!?

Jessie Berlin

Comment 4 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

Jia Pu

Comment 5 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.

Jessie Berlin

Comment 6 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!

Jia Pu

Comment 7 2011-05-12 15:32:47 PDT

Reopen since it was closed by mistake.

Jia Pu

Comment 8 2011-05-12 15:34:39 PDT

Created attachment 93357 [details] Patch (v2)

Jia Pu

Comment 9 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.

Jia Pu

Comment 10 2011-05-12 16:40:49 PDT

Created attachment 93365 [details] Patch (v3)

Darin Adler

Comment 11 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

Jia Pu

Comment 12 2011-05-12 17:32:47 PDT

Created attachment 93374 [details] Patch (v4) Fixed typo.

WebKit Commit Bot

Comment 13 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>

WebKit Commit Bot

Comment 14 2011-05-12 18:19:27 PDT

All reviewed patches have been landed. Closing bug.

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