Bug 66128 – A second form submission is not allowed if the first submission was canceled in an onbeforeunload handler.

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Andy Estes]

A second form submission is not allowed if the first submission was canceled in an onbeforeload handler.

[Andy Estes]

Created attachment 103751 [details]
Patch

[WebKit Review Bot]

Comment on attachment 103751 [details]
Patch

Attachment 103751 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9348573

[Andy Estes]

Created attachment 103752 [details]
Patch

[Jessie Berlin]

Comment on attachment 103751 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103751&action=review

Unofficial r=me! (not a reviewer yet)

> Tools/DumpRenderTree/LayoutTestController.cpp:2136
> +static JSValueRef allowRoundingHacksCallback(JSContextRef context, JSObjectRef, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)

Nit-pick: You mention in the ChangeLog that this is unrelated - it should really be in a separate patch.

> Tools/DumpRenderTree/LayoutTestController.cpp:2150
> +        shouldStayOnPageAfterHandlingBeforeUnload = JSValueToBoolean(context, arguments[0]);

This effectively makes the singular boolean argument optional. I think with a name like "setShouldStayOnPageAfterHandlingBeforeUnload", the argument should not be treated as optional and it should just bail early if an argument is not provided:

if (argumentCount < 1)
    return JSValueMakeUndefined(context);

> Tools/DumpRenderTree/win/UIDelegate.cpp:476
>      return E_NOTIMPL;

If this is implemented, why is it returning E_NOTIMPL instead of S_OK?

> Tools/WebKitTestRunner/TestController.cpp:110
> +    printf("CONFIRM NAVIGATION: %s\n", toSTD(message).c_str());

Do these changes to the output message mean that the expected results for any other tests need to be updated? (Ditto for the WK1 changes)

[Alexey Proskuryakov]

Is this a duplicate of bug 26211?

[Alexey Proskuryakov]

Or bug 25885?

[Andy Estes]

Yup, I think this is a dupe of bug 26211. It sounds like bug 25885 tracked multiple issues and the reporter spun off bug 26211 to track the onbeforeunload issue specifically. I'll dupe this bug and post a patch on bug 26211 with updates for Jessie's comments.

[Andy Estes]

(In reply to comment #4)
> (From update of attachment 103751 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103751&action=review
> 
> Unofficial r=me! (not a reviewer yet)

Unofficial thanks!

> 
> > Tools/DumpRenderTree/LayoutTestController.cpp:2136
> > +static JSValueRef allowRoundingHacksCallback(JSContextRef context, JSObjectRef, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
> 
> Nit-pick: You mention in the ChangeLog that this is unrelated - it should really be in a separate patch.
> 
> > Tools/DumpRenderTree/LayoutTestController.cpp:2150
> > +        shouldStayOnPageAfterHandlingBeforeUnload = JSValueToBoolean(context, arguments[0]);
> 
> This effectively makes the singular boolean argument optional. I think with a name like "setShouldStayOnPageAfterHandlingBeforeUnload", the argument should not be treated as optional and it should just bail early if an argument is not provided:

I agree.

> 
> if (argumentCount < 1)
>     return JSValueMakeUndefined(context);
> 
> > Tools/DumpRenderTree/win/UIDelegate.cpp:476
> >      return E_NOTIMPL;
> 
> If this is implemented, why is it returning E_NOTIMPL instead of S_OK?

It should return S_OK!

> 
> > Tools/WebKitTestRunner/TestController.cpp:110
> > +    printf("CONFIRM NAVIGATION: %s\n", toSTD(message).c_str());
> 
> Do these changes to the output message mean that the expected results for any other tests need to be updated? (Ditto for the WK1 changes)

No tests that I can run locally require updated results. For ports I can't run locally, I'll watch the bots and update results where needed.

Thanks for the careful review!

[Andy Estes]

*** This bug has been marked as a duplicate of bug 26211 ***