[Webkit-unassigned] [Bug 188696] beforeunload interoperability issues with a throwing return

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=188696

--- Comment #17 from PhistucK <phistuck at chromium.org> ---
(In reply to Darin Adler from comment #15)
Thank you for the review and sorry for wasting your time.
I agree with you that the patch is nearly ready for being committed.
The problem is that I do not have a suitable development environment (no macOS and my remote Linux - powered by Janitor - has Clang 5+, so WebKitGTK fails to build its dependencies), so I cannot either compile, manually test my code, or run the automated tests.
(See comment 3)


> Comment on attachment 347442 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347442&action=review
> 
> Thanks for contributing this patch. Seems like a good change to make! Almost
> there, but needs a little work. Need to add a test, and there’s a mistake in
> the patch that a good test would find.
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests .
> 
> Normally we’d say *why* there are no new tests.

I was too embarrassed to write, "Because, hell, I did not even test it manually". :P

> If there is a change in
> behavior, the project requires we add a test to demonstrate that the patch
> works. Can we construct a test? I think we should be able to do this by
> passing an object that shows an exception in its valueOf function. We should
> look at our existing beforeunload tests to see if one can be adapted for
> this purpose.

Yeah, makes sense, I felt more comfortable blindingly writing code than blindingly writing a test. :P

> Ideally another test will cover the case where the valueOf
> function does not return an exception and ensure that it is called exactly
> once.

Huh, would you have added/thought about adding that test if I did not write that mistake? Is there such a test for every case that implicitly/explicitly calls toString? If not, do you think there should be one for every case? It seems to me that this would inflate the test suite considerably (and maybe also should be handled at the compiled IDL binding level instead somehow).
I do not mind having such a test (of course it would have caught my mistake), I just wonder if you think such practice should be extremely widespread.
(I do understand my mistake exists/existed in the code base for other cases that you had to fix, which is probably what prompts the request for such a test)

> That would have caught the mistake in the patch that I mention below.
> 
> > Source/WebCore/bindings/js/JSEventListener.cpp:194
> > +                    reportException(exec, exception);
> 
> We might need something like the call to uncaughtExceptionInEventHandler
> above. Not sure if we need it. Might require a little research.

Without being able to compile/test, a research would be hard to commence... I might try to consult with some GTK folks about overcoming my Clang 5+ problem without downgrading Clang.

> 
> > Source/WebCore/bindings/js/JSEventListener.cpp:196
> > +                    handleBeforeUnloadEventReturnValue(downcast<BeforeUnloadEvent>(event), convert<IDLNullable<IDLDOMString>>(*exec, retval));
> 
> This needs to pass returnedString, and not compute it a second time.

Correct, I will fix this.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180820/da8b4e8a/attachment.html>