Bug 62940 – Disallow assigning into PassOwnArrayPtr, PassOwnPtr and PassRefPtr

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Anders Carlsson]

Disallow assigning into PassOwnArrayPtr, PassOwnPtr and PassRefPtr

[Anders Carlsson]

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).

[Anders Carlsson]

Created attachment 97707 [details]
Patch

[WebKit Review Bot]

Comment on attachment 97707 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 97707 [details]
Patch

Attachment 97707 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8874987

[Darin Adler]

Comment on attachment 97707 [details]
Patch

Anders, it looks like V8 bindings have some problems of this type, so this breaks the V8 build.

[Darin Adler]

This is a really great idea. We just have to do it with the changes to keep V8 bindings working.

[Anders Carlsson]

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

[Adam Barth]

I can sort out any V8 issues for you on Monday.

[Anders Carlsson]

(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...

[Darin Adler]

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.

[Darin Adler]

(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.

[Anders Carlsson]

Created attachment 97842 [details]
Patch

[Darin Adler]

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.

[Anders Carlsson]

Committed r89283: <http://trac.webkit.org/changeset/89283>