Summary: | Crash while uploading a PDF document on www.largefilesasap.com | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | George Stamatis <stamatgeorge> | ||||||||||
Component: | New Bugs | Assignee: | Kent Tamura <tkent> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Critical | CC: | ap, beidson, darin, tkent | ||||||||||
Priority: | P1 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Mac (Intel) | ||||||||||||
OS: | OS X 10.6 | ||||||||||||
Attachments: |
|
Description
George Stamatis
2010-03-28 08:21:36 PDT
Was using Safari Version 4.0.5 (6531.22.7, r56652) when it crashed *** Bug 36856 has been marked as a duplicate of this bug. *** (In reply to comment #2) > http://trac.webkit.org/changeset/56439 ? Yup. I'm working on this now, should be an easy fix (I have more analysis in the dupe if anyone is interested.) Fundamental problem is that the FileChooser might call into its client (RenderFileUploadControl ) during the constructor, but RenderFileUploadControl needs to pass the FileChooser off to *its* client (WebChromeClient), but it doesn't yet *have* the FileChooser to pass off. We'll either have to: A - Make the logic in void FileChooser::chooseFiles(const Vector<String>& filenames) kind of gross B - Make constructing a FileChooser a 2-stage operation (create followed by and init) C - Revert r56439 as its really just a code shuffle and didn't change any functionality. (In reply to comment #5) > Fundamental problem is that the FileChooser might call into its client > (RenderFileUploadControl ) during the constructor, but RenderFileUploadControl > needs to pass the FileChooser off to *its* client (WebChromeClient), but it > doesn't yet *have* the FileChooser to pass off. > > We'll either have to: > A - Make the logic in void FileChooser::chooseFiles(const Vector<String>& > filenames) kind of gross > B - Make constructing a FileChooser a 2-stage operation (create followed by and > init) > C - Revert r56439 as its really just a code shuffle and didn't change any > functionality. And quite honestly, my vote goes to C. Created attachment 52118 [details]
Patch
Comment on attachment 52118 [details] Patch Quite honestly, this is my least favorite of the three approaches I suggested. IMHO, 2 stage construction is not a great pattern unless there's no alternatives. I added this comment to the original bug (https://bugs.webkit.org/show_bug.cgi?id=35072): --- Since this patch: 1 - Didn't change functionality 2 - The approach was generally not liked by the reviewers. 3 - In general, round tripping between WebCore and WebKit is something we try to *reduce* ...I think it needs to be rolled out. --- Can you convince me why round tripping between WebCore and WebKit for 5+ different platforms was better than the previous solution that kept things entirely in WebCore except for the lone platform that needed the client interface? I know Darin expressed disdain for the duplication of the API call in https://bugs.webkit.org/show_bug.cgi?id=32054 because it would make the code more complicated and harder to maintain in the long run. But is the current state of things actually *better*? I think it's crazy that WebCore always calls out to the client interfaces for 5+ different platforms, and most of them call straight back into WebCore. This makes the code harder to follow and maintain, not easier. This comment reply moved from https://bugs.webkit.org/show_bug.cgi?id=35072 because it's more relevant to this discussion: (In reply to comment #17) > (In reply to comment #16) > > This patch (http://trac.webkit.org/changeset/56439) caused a major regression > > (https://bugs.webkit.org/show_bug.cgi?id=36723) > > > > Since: > > 1 - It didn't change functionality > > 2 - The approach was generally not liked. > > 3 - In general, round tripping between WebCore and WebKit is something we try > > to *reduce* > > > > ...I think it needs to be rolled out. > > Brady, I already post a fix for the regression. Please don't roll it out. > Even if we rolled it out, the problem would not be solved with platforms of > which Icon::iconForFiles() returns 0. Okay, that's true. The mere existence of the client interface means that if a platform returns 0 for Icon::iconForFiles(), we'd still have this problem. I'm still far from convinced that forcing all platforms through this client interface is the right thing to do when most only need what WebCore already knows how to do, and that same code is copied 4+ times. Comment on attachment 52118 [details] Patch Knowing that we'd have this problem whether or not we roll out http://trac.webkit.org/changeset/56439 , I'll review this patch differently. I like the layout test and I like the asserts in RenderFileUploadControl.cpp, but I'm not convinced of the change to 2 step construction for the FileChooser. There is exactly 1 user of FileChooser right now, so it's easy to get the 2 step construction right. But this pattern might be difficult to maintain going forward. I urge you to consider working out a clean way to remove loadIcon() from the constructor but *not* add the requirement to manually call loadIcon() afterwards. But, r+ anyways. Actually I don't like the 2 step construction too. I'll make another patch. Created attachment 52120 [details]
Patch
Comment on attachment 52120 [details] Patch Thanks for making this change. > @@ -79,13 +81,13 @@ void FileChooser::chooseFiles(const Vector<String>& filenames) > void FileChooser::loadIcon() > { > if (m_filenames.size() && m_client) > - m_client->chooseIconForFiles(m_filenames); > + m_client->chooseIconForFiles(this, m_filenames); > } > diff --git a/WebCore/rendering/RenderFileUploadControl.cpp b/WebCore/rendering/RenderFileUploadControl.cpp > index 14d126d..a66b118 100644 > --- a/WebCore/rendering/RenderFileUploadControl.cpp > +++ b/WebCore/rendering/RenderFileUploadControl.cpp > @@ -114,10 +114,10 @@ String RenderFileUploadControl::acceptTypes() > -void RenderFileUploadControl::chooseIconForFiles(const Vector<String>& filenames) > +void RenderFileUploadControl::chooseIconForFiles(PassRefPtr<FileChooser> chooser, const Vector<String>& filenames) > { > if (Chrome* chromePointer = chrome()) > - chromePointer->chooseIconForFiles(filenames, m_fileChooser); > + chromePointer->chooseIconForFiles(filenames, chooser); > } > diff --git a/WebCore/rendering/RenderFileUploadControl.h b/WebCore/rendering/RenderFileUploadControl.h > index 99dd35c..9714db1 100644 > --- a/WebCore/rendering/RenderFileUploadControl.h > +++ b/WebCore/rendering/RenderFileUploadControl.h > @@ -61,7 +61,7 @@ private: > - void chooseIconForFiles(const Vector<String>&); > + void chooseIconForFiles(PassRefPtr<FileChooser>, const Vector<String>&); PassRefPtr is wrong here. In the original bug, Darin pointed this out (https://bugs.webkit.org/show_bug.cgi?id=35072#c8) and you described why you thought this qualified (https://bugs.webkit.org/show_bug.cgi?id=35072#c12), and I think someone should've followed up on this. PassRefPtr is actually about *transferring* ownership. It's meant to be used when you're transferring from one RefPtr to another RefPtr. In that case, it zeroes out the old RefPtr without decrementing the ref count. It then assigns the raw pointer to the new RefPtr without incrementing the ref count. This allows ownership transfer without (surprisingly expensive) ref count churn. Creating this PassRefPtr from the raw pointer *always* bumps the ref count, even if the client isn't interested in hanging on to the object. Then the ~PassRefPtr decrements the ref count, even when the client never cared. That's the churn that PassRefPtr is actually designed to avoid. This case wasn't ever actually *transferring* ownership. While it's true that Chromium needs to ref the object to hang on to it, the RenderFileUploadControl doesn't actually *give up* its ref. In this regard, the PassRefPtr is wrong in all of the client methods originally implemented in each of the WebKits. This really can be a raw pointer. In the common platform (all but Chromium) that don't need a ref and that synchronously call back into WebCore, not ref'ing is correct. In the Chromium case, it has to bump the ref count anyways. I'm tempted to ask you to update all of the client methods to be raw pointers, but since this isn't really a hot code path, I won't insist. However, I will insist that this new method be a raw pointer, so we don't carry the mistake all the way to the core. :) Created attachment 52122 [details]
Patch
(In reply to comment #13) > PassRefPtr is wrong here. Thank you for the explanation. I agree that we don't need to use PassRefPtr for this change, and for Chrome::chooseIconForFiles() too. I'll address Chrome::chooseIconForFiles() when we have a conclusion for Icon interface change. Comment on attachment 52122 [details] Patch > - m_client->chooseIconForFiles(m_filenames); > + m_client->chooseIconForFiles(*this, m_filenames); > - virtual void chooseIconForFiles(const Vector<String>&) = 0; > + virtual void chooseIconForFiles(FileChooser&, const Vector<String>&) = 0; > virtual ~FileChooserClient(); > > -void RenderFileUploadControl::chooseIconForFiles(const Vector<String>& filenames) > +void RenderFileUploadControl::chooseIconForFiles(FileChooser& chooser, const Vector<String>& filenames) > { > if (Chrome* chromePointer = chrome()) > - chromePointer->chooseIconForFiles(filenames, m_fileChooser); > + chromePointer->chooseIconForFiles(filenames, &chooser); > - void chooseIconForFiles(const Vector<String>&); > + void chooseIconForFiles(FileChooser&, const Vector<String>&); We normally only use references for values stack values, and I'm not aware of anywhere where we use them for heap-only objects. This really can be a raw pointer: void chooseIconForFiles(FileChooser*, const Vector<String>&); Created attachment 52123 [details]
Patch
Comment on attachment 52123 [details] Patch > @@ -44,7 +45,7 @@ public: > virtual void repaint() = 0; > virtual bool allowsMultipleFiles() = 0; > virtual String acceptTypes() = 0; > - virtual void chooseIconForFiles(const Vector<String>&) = 0; > + virtual void chooseIconForFiles(FileChooser*, const Vector<String>&) = 0; > virtual ~FileChooserClient(); > }; > This is a better change, anyways - we like telling clients which object it is they're working on, because one object can be the client for multiple clientees. r+! Landed as r56824. |