Disallow assigning into PassOwnArrayPtr, PassOwnPtr and PassRefPtr
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).
Created attachment 97707 [details] Patch
Comment on attachment 97707 [details] Patch Attachment 97707 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8872956
Comment on attachment 97707 [details] Patch Attachment 97707 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8874987
Comment on attachment 97707 [details] Patch Anders, it looks like V8 bindings have some problems of this type, so this breaks the V8 build.
This is a really great idea. We just have to do it with the changes to keep V8 bindings working.
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
I can sort out any V8 issues for you on Monday.
(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...
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.
(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.
Created attachment 97842 [details] Patch
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.
Committed r89283: <http://trac.webkit.org/changeset/89283>