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: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Mac (Intel) | ||||||||||||
OS: | OS X 10.6 | ||||||||||||
Attachments: |
|
Description
Jia Pu
2011-05-12 11:14:01 PDT
Created attachment 93316 [details]
Patch (v1)
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 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 on attachment 93316 [details] Patch (v1) Thanks for the review, Darin! Committed in http://trac.webkit.org/changeset/86371 (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. (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! Reopen since it was closed by mistake. Created attachment 93357 [details]
Patch (v2)
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.
Created attachment 93365 [details]
Patch (v3)
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 Created attachment 93374 [details]
Patch (v4)
Fixed typo.
Comment on attachment 93374 [details] Patch (v4) Clearing flags on attachment: 93374 Committed r86406: <http://trac.webkit.org/changeset/86406> All reviewed patches have been landed. Closing bug. |