Summary: | [commit+] [v8] check if proxy is present before invoking a handler | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | anton muhin <antonm> | ||||||
Component: | WebKit Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, vitalyr | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
anton muhin
2009-07-31 09:03:56 PDT
Created attachment 33878 [details] Initial version To be honest, I would guess the code expects always not NULL proxy here as listeners are enabled. (but I think in that case it should be documented with ASSERT). So I am by no means insisting on this patch. One thing that stroke me is presence of GoogleUpdateClient in the call stack (see http://crash/reportdetail?reportid=19d99906cbceb8a7&product=Chrome&version=3.0.196.0&date=&signature=WebCore::V8Proxy::callFunction%28v8::Handle). My immediate reaction is something wasn't initialized properly. And I'll be on vacation two next weeks. Comment on attachment 33878 [details] Initial version > Index: third_party/WebKit/WebCore/ChangeLog > +2009-07-31 Anton Muhin <antonm@chromium.org> > + > + Reviewed by NOBODY (OOPS!). > + Please add: Bug Title Bug Link "prepare-ChangeLog --bug #" will do this for you. > + Do not invoke handler functio if proxy is NULL (that would lead to NPE anyway). typo: functio Change NULL to 0 (since WebKit doesn't use NULL). NPE is not an abbreviation that I'm familiar with but I guess you mean crash/access violation. It would be good to write it in a longer form without using an abbreviation. Also, this ChangeLog explains what but not why. Please add why. Can there be a layout test for whatever problem was encountered? If so, please add one. If not, why not? Created attachment 33893 [details]
Next iteration
(In reply to comment #2) > (From update of attachment 33878 [details]) > > Index: third_party/WebKit/WebCore/ChangeLog > > +2009-07-31 Anton Muhin <antonm@chromium.org> > > + > > + Reviewed by NOBODY (OOPS!). > > + > > Please add: > > Bug Title > Bug Link > > "prepare-ChangeLog --bug #" will do this for you. Hopefully done. Thanks a lot for pointing out a flag. Could this information be added to http://webkit.org/coding/contributing.html (just asking) > > > + Do not invoke handler functio if proxy is NULL (that would lead to NPE anyway). > > typo: functio Tnx, done. > > Change NULL to 0 (since WebKit doesn't use NULL). > > NPE is not an abbreviation that I'm familiar with but I guess you mean > crash/access violation. It would be good to write it in a longer form without > using an abbreviation. Hopefully done. > > Also, this ChangeLog explains what but not why. Please add why. The problem is I don't know why (that's why I think we could drop this patch)---I think error is higher level, but I don't know enough to find the proper place. > > > Can there be a layout test for whatever problem was encountered? > If so, please add one. > If not, why not? See above. Sorry for repetition, but you, David, may know better person to track this down---I believe the problem is in higher levels (but in that case I'd like to see ASSERT here instead my poor if)---I can understand that if event listener could be invoked, there should be proxy. However, I am definitely not a person to understand every minutae of the stuff. Comment on attachment 33893 [details]
Next iteration
Thanks for the patch. Can you file a bug to investigate whether we're making a mistake at a higher level?
As for modifying contributing.html, it's in SVN under WebKitSite. Feel free to file a bug and propose a patch. :)
In the future, please create patches with bugzilla-tool or svn-create-patch. This patch isn't correctly based, so we can't use our automatic tools. :( Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/bindings/v8/custom/V8CustomEventListener.cpp Committed r46689 M WebCore/ChangeLog M WebCore/bindings/v8/custom/V8CustomEventListener.cpp r46689 = 0fe2074611f24744d6dc1a6d37d9bb5f7ab66d10 (trunk) No changes between current HEAD and refs/remotes/trunk Resetting to the latest refs/remotes/trunk http://trac.webkit.org/changeset/46689 |