Bug 60712 – Regression caused by changeset 86281

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Jia Pu]

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

[Jia Pu]

Created attachment 93316 [details]
Patch (v1)

[Jia Pu]

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 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 on attachment 93316 [details]
Patch (v1)

Thanks for the review, Darin! Committed in http://trac.webkit.org/changeset/86371

[Jia Pu]

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

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

Reopen since it was closed by mistake.

[Jia Pu]

Created attachment 93357 [details]
Patch (v2)

[Jia Pu]

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]

Created attachment 93365 [details]
Patch (v3)

[Darin Adler]

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]

Created attachment 93374 [details]
Patch (v4)

Fixed typo.

[WebKit Commit Bot]

Comment on attachment 93374 [details]
Patch (v4)

Clearing flags on attachment: 93374

Committed r86406: <http://trac.webkit.org/changeset/86406>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.