Bug 62940

Summary: Disallow assigning into PassOwnArrayPtr, PassOwnPtr and PassRefPtr
Product: WebKit Reporter: Anders Carlsson <andersca>
Component: Web Template FrameworkAssignee: Anders Carlsson <andersca>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andersca, darin, dglazkov, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch darin: review+

Description Anders Carlsson 2011-06-18 15:10:09 PDT
Disallow assigning into PassOwnArrayPtr, PassOwnPtr and PassRefPtr
Comment 1 Anders Carlsson 2011-06-18 15:13:18 PDT
In order to tighten our "pass a reference" style smart pointers, I think we should disallow assigning into them; this will hopefully encourage people to only use them for parameters and return values, and should also help reduce the bugs that will be found by https://bugs.webkit.org/show_bug.cgi?id=44989 (which I have a separate patch for).
Comment 2 Anders Carlsson 2011-06-18 15:19:21 PDT
Created attachment 97707 [details]
Patch
Comment 3 WebKit Review Bot 2011-06-18 15:37:52 PDT
Comment on attachment 97707 [details]
Patch

Attachment 97707 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8872956
Comment 4 WebKit Review Bot 2011-06-18 16:51:16 PDT
Comment on attachment 97707 [details]
Patch

Attachment 97707 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8874987
Comment 5 Darin Adler 2011-06-18 17:29:20 PDT
Comment on attachment 97707 [details]
Patch

Anders, it looks like V8 bindings have some problems of this type, so this breaks the V8 build.
Comment 6 Darin Adler 2011-06-18 17:37:35 PDT
This is a really great idea. We just have to do it with the changes to keep V8 bindings working.
Comment 7 Anders Carlsson 2011-06-18 19:13:48 PDT
The offending line in V8HTMLTextAreaElement.cpp is:

    EXCEPTION_BLOCK(PassRefPtr<DOMStringList>, suggestions, v8ValueToWebCoreDOMStringList(args[2]));

in addSpellcheckRangeCallback.

EXCEPTION_BLOCK is a macro that looks like:

#define EXCEPTION_BLOCK(type, var, value) \
    type var;                             \
    {                                     \
        v8::TryCatch block;               \
        var = (value);                    \
        if (block.HasCaught())            \
            return block.ReThrow();       \
    }


This is a V8 wrapper around the HTMLTextAreaElement member function

    void addSpellcheckRange(unsigned long start, unsigned long length, RefPtr<DOMStringList>, unsigned short options = 0);

which the IDL file specifies as:

        void addSpellcheckRange(in unsigned long start, in unsigned long length, in [Optional] DOMStringList suggestions, in [Optional] unsigned short options);

Now there's code in the V8 bindings generator (GetNativeType to be exact) to always return PassRefPtr<DOMStringList> for DOMStringList parameters:

    return "PassRefPtr<DOMStringList>" if $type eq "DOMStringList" and $isParameter;

That doesn't seem necessary to me, but was added in https://bugs.webkit.org/show_bug.cgi?id=56950
Comment 8 Adam Barth 2011-06-18 22:11:44 PDT
I can sort out any V8 issues for you on Monday.
Comment 9 Anders Carlsson 2011-06-19 08:36:39 PDT
(In reply to comment #8)
> I can sort out any V8 issues for you on Monday.

That'd be great. I filed https://bugs.webkit.org/show_bug.cgi?id=62947 to test a patch that simply makes the V8 bindings stop using PassRefPtr<DOMStringList> for DOMStringList parameters, and that seems to have built fine...
Comment 10 Darin Adler 2011-06-19 13:02:53 PDT
Here’s my take on those bindings quirks:

Those DOM spellcheck range functions are wrong to use RefPtr for their arguments. I just noticed that mistake separately. They should take PassRefPtr or a raw pointer. We never use RefPtr for an argument!

The binding script is wrong to ever explicitly a PassRefPtr. I know of no downside to passing a raw pointer at the bindings level and letting the compiler generate the conversion to PassRefPtr.
Comment 11 Darin Adler 2011-06-19 13:13:06 PDT
(In reply to comment #10)
> Those DOM spellcheck range functions are wrong to use RefPtr for their arguments. I just noticed that mistake separately. They should take PassRefPtr or a raw pointer. We never use RefPtr for an argument!

I filed bug 62954 about this.
Comment 12 Anders Carlsson 2011-06-20 12:46:36 PDT
Created attachment 97842 [details]
Patch
Comment 13 Darin Adler 2011-06-20 12:50:10 PDT
Comment on attachment 97842 [details]
Patch

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

> Source/WebCore/editing/htmlediting.cpp:892
> +    RefPtr<Node> tabTextNode = prpTabTextNode;

I would have put this at the top of the function instead of here.
Comment 14 Anders Carlsson 2011-06-20 13:22:01 PDT
Committed r89283: <http://trac.webkit.org/changeset/89283>