Bug 50306 – REGRESSION: WebChromeClient::shouldReplaceWithGeneratedFileForUpload() uses an uninitialized result value (breaks form submission with clang-built WebKit)

• Component: Forms
• Status: RESOLVED FIXED

[Jon]

Building r72983 with clang Apple clang version 2.0 (tags/Apple/clang-121.4) (based on LLVM 2.9svn) works, but it seems to produce a failure with multipart/form-data forms. One that doesn't exist when building with gcc or llvm-gcc. And that doesn't seem to affect other form enctype's. The page I've linked isn't mine but has several form tests. All of them work correctly in shipping Safari but the multipart/form-data one fails when use the same Safari with WebKit built with clang. Ironically this bug also affects the WebKit bug entry form, so I have to submit this bug from shipping Safari. Strangely, some of the forms I've encountered with this issue can be fixed by removing the enctype parameter from the form declaration using the inspector. However, the one here isn't fixed, possibly because it actually needs the enctype to function properly. Whatever the case, removing the enctype parameter from the page I linked at least lets the form's submit button work, even if the results then differ from shipping Safari.

[Jon]

This is still occuring for me using r77578 and Apple clang version 2.0 (tags/Apple/clang-134) (based on LLVM 2.9svn). The page I linked has a good test. Can someone take a look? I'd like to know if I've been hallucinating this bizarre bug for the past few months.

[Jon]

During this failure Safari throws an exception. *** -[NSFileManager fileSystemRepresentationWithPath:]: nil or empty path argument

Here's the stack:


#0  0x00007fff8725f0da in objc_exception_throw ()
#1  0x00007fff840fa5d7 in +[NSException raise:format:arguments:] ()
#2  0x00007fff840fa564 in +[NSException raise:format:] ()
#3  0x00007fff8187d226 in -[NSFileManager fileSystemRepresentationWithPath:] ()
#4  0x0000000100159bf6 in ?? ()
#5  0x000000010015af45 in ?? ()
#6  0x00007fff840cf96c in __invoking___ ()
#7  0x00007fff840cf83d in -[NSInvocation invoke] ()
#8  0x00007fff840eb711 in -[NSInvocation invokeWithTarget:] ()
#9  0x00007fff840cc98c in ___forwarding___ ()
#10 0x00007fff840c8a68 in __forwarding_prep_0___ ()
#11 0x0000000100a11bf0 in WebChromeClient::shouldReplaceWithGeneratedFileForUpload (this=0x102e1b1f0, path=@0x119547058, generatedFilename=@0x7fff5fbfe480) at /Users/jshier/Programming/webkit/Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:729
#12 0x0000000100ef4489 in WebCore::FormData::appendKeyValuePairItems (this=0x11b5ffd80, list=@0x11b590060, encoding=@0x11b590060, document=0x119362000) at /Users/jshier/Programming/webkit/Source/WebCore/platform/network/FormData.cpp:217
#13 0x0000000100ef4c2a in WebCore::FormData::createMultiPart (list=@0x11b590060, encoding=@0x11b590060, document=0x119362000) at /Users/jshier/Programming/webkit/Source/WebCore/platform/network/FormData.cpp:112
#14 0x0000000100efaae8 in WebCore::FormSubmission::create (form=0x11951f200, attributes=@0x3, trigger=WebCore::NotSubmittedByJavaScript) at /Users/jshier/Programming/webkit/Source/WebCore/loader/FormSubmission.cpp:196
#15 0x0000000100fa59c9 in WebCore::HTMLFormElement::submit (this=0x11951f200, event=0x11b564b58, formSubmissionTrigger=WebCore::NotSubmittedByJavaScript) at /Users/jshier/Programming/webkit/Source/WebCore/html/HTMLFormElement.cpp:337
#16 0x0000000100fa512e in WebCore::HTMLFormElement::prepareForSubmission (this=0x11951f200, event=0x11b564b58) at /Users/jshier/Programming/webkit/Source/WebCore/html/HTMLFormElement.cpp:285
#17 0x000000010150db01 in WebCore::SubmitInputType::handleDOMActivateEvent (this=0x11951f200, event=0x11b564b58) at /Users/jshier/Programming/webkit/Source/WebCore/html/SubmitInputType.cpp:73
#18 0x0000000100fb103e in WebCore::HTMLInputElement::defaultEventHandler (this=0x1190e1540, evt=0x11b564b58) at /Users/jshier/Programming/webkit/Source/WebCore/html/HTMLInputElement.cpp:1009
#19 0x000000010139532f in WebCore::Node::dispatchGenericEvent (this=0x11951f200) at /Users/jshier/Programming/webkit/Source/WebCore/dom/Node.cpp:2711
#20 0x0000000101394ecc in WebCore::Node::dispatchEvent (this=0x11951f200) at /Users/jshier/Programming/webkit/Source/WebCore/dom/Node.cpp:2607
#21 0x00000001014b6d27 in WebCore::ScopedEventQueue::dispatchEvent (this=0x11951f200) at /Users/jshier/Programming/webkit/Source/WebCore/dom/ScopedEventQueue.cpp:79
#22 0x00000001014b6cb1 in WebCore::ScopedEventQueue::enqueueEvent (this=0x11951f200) at /Users/jshier/Programming/webkit/Source/WebCore/dom/ScopedEventQueue.cpp:64
#23 0x00000001013955e5 in WebCore::Node::dispatchScopedEvent (this=0x11951f200) at /Users/jshier/Programming/webkit/Source/WebCore/dom/Node.cpp:2615
#24 0x000000010139594d in WebCore::Node::dispatchUIEvent (this=0x1190e1540, eventType=@0x102cd9d88, detail=1) at /Users/jshier/Programming/webkit/Source/WebCore/dom/Node.cpp:2761
#25 0x0000000101396ed1 in WebCore::Node::defaultEventHandler (this=0x1190e1540, event=0x11ca73f00) at /Users/jshier/Programming/webkit/Source/WebCore/dom/Node.cpp:2993
#26 0x0000000100fb1342 in WebCore::HTMLInputElement::defaultEventHandler (this=0x1190e1540, evt=0x11ca73f00) at /Users/jshier/Programming/webkit/Source/WebCore/html/HTMLInputElement.cpp:1069
#27 0x000000010139532f in WebCore::Node::dispatchGenericEvent (this=0x1190e1540) at /Users/jshier/Programming/webkit/Source/WebCore/dom/Node.cpp:2711
#28 0x0000000101394ecc in WebCore::Node::dispatchEvent (this=0x1190e1540) at /Users/jshier/Programming/webkit/Source/WebCore/dom/Node.cpp:2607
#29 0x0000000101395ebf in WebCore::Node::dispatchMouseEvent (this=0x1190e1540, eventType=@0x102cd9b60, button=0, detail=1, pageX=71, pageY=71, relatedTargetArg=0x0, screenY=707, screenX=71) at /Users/jshier/Programming/webkit/Source/WebCore/dom/Node.cpp:2885
#30 0x0000000101395b3c in WebCore::Node::dispatchMouseEvent (this=0x1190e1540, event=@0x7fff5fbfedb8, eventType=@0x102cd9b60, detail=1, relatedTarget=0x0) at /Users/jshier/Programming/webkit/Source/WebCore/dom/Node.cpp:2791
#31 0x0000000100ec6d36 in WebCore::EventHandler::dispatchMouseEvent (this=0x102cb21a8, eventType=@0x102cd9b60, targetNode=0x102cd9b60, clickCount=1, mouseEvent=@0x7fff5fbfef60) at /Users/jshier/Programming/webkit/Source/WebCore/page/EventHandler.cpp:1911
#32 0x0000000100ec844a in WebCore::EventHandler::handleMouseReleaseEvent (this=0x1190e1540, mouseEvent=@0x7fff5fbfef60) at /Users/jshier/Programming/webkit/Source/WebCore/page/EventHandler.cpp:1624
#33 0x0000000100eccbbf in WebCore::EventHandler::mouseUp (this=0x1190e1540, event=0x7fff5fbfef60) at /Users/jshier/Programming/webkit/Source/WebCore/page/mac/EventHandlerMac.mm:546
#34 0x0000000100a47d04 in -[WebHTMLView mouseUp:] (self=0x102e1d550, _cmd=0x102cd9b60, event=0x10298c130) at /Users/jshier/Programming/webkit/Source/WebKit/mac/WebView/WebHTMLView.mm:3764
#35 0x00007fff8312f3d9 in -[NSWindow sendEvent:] ()
#36 0x0000000100042489 in ?? ()
#37 0x0000000100042416 in ?? ()
#38 0x00007fff83064a86 in -[NSApplication sendEvent:] ()
#39 0x0000000100039146 in ?? ()
#40 0x00007fff82ffb4da in -[NSApplication run] ()
#41 0x00007fff82ff41a8 in NSApplicationMain ()
#42 0x000000010000a1c0 in ?? ()

Perhaps clang is intializing a variable differently than gcc?

[Alexey Proskuryakov]

I don't know how to build WebKit with clang, but I think that I know what's going on. We do call -[NSFileManager fileSystemRepresentationWithPath:] with a nil argument, it raises an exception, and we end up returning from -[_WebSafeForwarder forwardInvocation:] with an uninitialized result. 

This use of uninitialized result is a regression from shipping WebKit, I can observe it with a gcc build - even though the form doesn't fail to submit for me.

[Alexey Proskuryakov]

<rdar://problem/8976152>

[Alexey Proskuryakov]

I didn't bisect builds to confirm, but this is likely due to r66452, which removed a check for path length in FormData::appendKeyValuePairItems().

[Alexey Proskuryakov]

> *** -[NSFileManager fileSystemRepresentationWithPath:]: nil or empty path argument

Jon, do you also see the following line?

*** WebKit discarded an uncaught exception in the webView:shouldReplaceUploadFile:usingGeneratedFilename: delegate:

[Alexey Proskuryakov]

Created attachment 81747 [details]
proposed fix

I'm concerned that _WebSafeForwarder may have been bypassed somehow, or worked incorrectly. But I can't check that, and this patch should fix form submission.

[Jon]

(In reply to comment #6)
> > *** -[NSFileManager fileSystemRepresentationWithPath:]: nil or empty path argument
> 
> Jon, do you also see the following line?
> 
> *** WebKit discarded an uncaught exception in the webView:shouldReplaceUploadFile:usingGeneratedFilename: delegate:

No, the NSFileManager exception is the only output I see. I'll try your patch.

[Jon]

Patch works, as I'm now able to submit this Bugzilla form and comment on the bug. :)

[Alexey Proskuryakov]

> No, the NSFileManager exception is the only output I see.

That's confusing. It likely means that _WebSafeForwarder doesn't work, which is a big problem.

> Patch works, as I'm now able to submit this Bugzilla form and comment on the bug. :)

The problem should have only affected forms with file inputs - I don't think that commenting was broken.

[Jon]

It affected all forms that were multipart/form-data, whether a file was attached or not. I've been building WebKit with clang with the Xcode 4 previews.

[Darin Adler]

Comment on attachment 81747 [details]
proposed fix

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

> Source/WebCore/ChangeLog:24
> [phone]  Alexey Proskuryakov  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Need a short description and bug URL (OOPS!)
> +
> +        No new tests. (OOPS!)
> +
> +        * platform/network/FormData.cpp:
> +        (WebCore::FormData::appendKeyValuePairItems):
> +

Double change log.

[Alexey Proskuryakov]

Comment on attachment 81747 [details]
proposed fix

Will need to upload a new patch due to double change log...

[Alexey Proskuryakov]

Created attachment 81883 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 81883 [details]
patch for landing

Clearing flags on attachment: 81883

Committed r78202: <http://trac.webkit.org/changeset/78202>