[commit+] [v8] check if proxy is present before invoking a handler

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[anton muhin]

There is NPE crash (http://crash/reportdetail?reportid=19d99906cbceb8a7&product=Chrome&version=3.0.196.0&date=&signature=WebCore::V8Proxy::callFunction%28v8::Handle) which is most probably due NULL proxy.

[anton muhin]

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.

[David Levin]

Comment on attachment 33878 [details]
Initial version

> Index: third_party/WebKit/WebCore/ChangeLog
> [phone]  Anton Muhin  <[e-mail]>
> +
> +        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?

[anton muhin]

Created attachment 33893 [details]
Next iteration

[anton muhin]

(In reply to comment #2)
> (From update of attachment 33878 [details])
> > Index: third_party/WebKit/WebCore/ChangeLog
> > [phone]  Anton Muhin  <[e-mail]>
> > +
> > +        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.

[Adam Barth]

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.  :)

[Adam Barth]

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.  :(

[Adam Barth]

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