Bug 52867 – OwnArraryPtr.h uses deleteOwnedPtr but doesn’t include OwnPtrCommon.h

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Chromium clang build is failing with the following error:

> In file included from
> third_party/WebKit/Source/WebCore/html/HTMLFrameSetElement.h:27:
> third_party/WebKit/Source/JavaScriptCore/wtf/OwnArrayPtr.h:123:5:error:
> use of undeclared identifier 'deleteOwnedPtr'
>    deleteOwnedPtr(oldPtr);
>    ^
> third_party/WebKit/Source/WebCore/html/HTMLFrameSetElement.cpp:78:26:
> note: in instantiation of member function
> 'WTF::OwnArrayPtr<WebCore::Length>::set' requested here
>            m_rowLengths.set(newLengthArray(attr->value().string(),
> m_totalRows));
>                         ^
> 1 error generated.

The problem is that OwnArrayPtr::set calls deleteOwnedPtr but doesn't include OwnPtrCommon.h.  We should either include the header or should call deleteOwnedArrayPtr.

[Ryosuke Niwa]

Created attachment 79693 [details]
Include OwnPtrCommon.h in OwnArrayPtr.h

Reading the comment:

// Remove this once we make all WebKit code compatible with stricter rules about OwnArrayPtr.
#define LOOSE_OWN_ARRAY_PTR

the correct thing to do here seems to include OwnPtrCommon.h.

[Maciej Stachowiak]

Comment on attachment 79693 [details]
Include OwnPtrCommon.h in OwnArrayPtr.h

r=me

[Ryosuke Niwa]

(In reply to comment #2)
> (From update of attachment 79693 [details])
> r=me

Thanks for the review!

[Ryosuke Niwa]

Comment on attachment 79693 [details]
Include OwnPtrCommon.h in OwnArrayPtr.h

Clearing flags on attachment: 79693

Committed r76334: <http://trac.webkit.org/changeset/76334>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

This was the wrong fix. OwnArrayPtr<T>::set needs to use deletedOwnedArrayPtr. Or it needs to be removed.

[Ryosuke Niwa]

(In reply to comment #6)
> This was the wrong fix. OwnArrayPtr<T>::set needs to use deletedOwnedArrayPtr. Or it needs to be removed.

Ok, let's fix then!

[Darin Adler]

(In reply to comment #7)
> (In reply to comment #6)
> > This was the wrong fix. OwnArrayPtr<T>::set needs to use deletedOwnedArrayPtr. Or it needs to be removed.
> 
> Ok, let's fix then!

My preference would be to remove it, if we can quickly find the callers using it and change them to use adoptPtr instead.

[Ryosuke Niwa]

Let me do this over the weekend (hopefully).

[Ryosuke Niwa]

Created attachment 79827 [details]
get rid of set

[WebKit Review Bot]

Attachment 79827 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7521251

[Build Bot]

Attachment 79827 [details] did not build on win:
Build output: http://queues.webkit.org/results/7622260

[Ryosuke Niwa]

Created attachment 79830 [details]
fixed platform code

[WebKit Review Bot]

Attachment 79830 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7510268

[Build Bot]

Attachment 79830 [details] did not build on win:
Build output: http://queues.webkit.org/results/7596258

[Ryosuke Niwa]

Created attachment 79834 [details]
more port specific fixes

[WebKit Review Bot]

Attachment 79834 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7561293

[Build Bot]

Attachment 79834 [details] did not build on win:
Build output: http://queues.webkit.org/results/7513275

[Ryosuke Niwa]

Created attachment 79839 [details]
another attempt

[WebKit Review Bot]

Attachment 79839 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7556269

[Build Bot]

Attachment 79839 [details] did not build on win:
Build output: http://queues.webkit.org/results/7536279

[Ryosuke Niwa]

Created attachment 79854 [details]
yet another attempt

[WebKit Review Bot]

Attachment 79854 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7505249

[Ryosuke Niwa]

Created attachment 79861 [details]
fixed v8 binding

[WebKit Review Bot]

Attachment 79861 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7626241

[Ryosuke Niwa]

Created attachment 81438 [details]
fixed cr-mac build

[Early Warning System Bot]

Attachment 81438 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7700733

[Ryosuke Niwa]

(In reply to comment #27)
> Attachment 81438 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/7700733

This is ews flakinss. kling kindly verified that it builds on qt.

[Darin Adler]

Comment on attachment 81438 [details]
fixed cr-mac build

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

Exciting to see this. Looking forward to it landing. review- because of the JavaScriptCore registers issues.

> Source/JavaScriptCore/runtime/Arguments.h:232
>          Register* registerArray = new Register[registerArraySize];
>          memcpy(registerArray, d->registers - registerOffset, registerArraySize * sizeof(Register));
> -        d->registerArray.set(registerArray);
> +        d->registerArray = registerArray;
>          d->registers = registerArray + registerOffset;

Needs an adoptArrayPtr. Probably won’t compile. I think this is may be why EWS bots are failing to build.

> Source/JavaScriptCore/runtime/JSVariableObject.h:165
>      inline void JSVariableObject::setRegisters(Register* registers, Register* registerArray)
>      {
>          ASSERT(registerArray != d->registerArray.get());
> -        d->registerArray.set(registerArray);
> +        d->registerArray = registerArray;
>          d->registers = registers;
>      }

Needs PassOwnArrayPtr. Same as above.

[Ryosuke Niwa]

(In reply to comment #29)
> (From update of attachment 81438 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81438&action=review
> 
> Exciting to see this. Looking forward to it landing. review- because of the JavaScriptCore registers issues.

Thanks for the review.

> > Source/JavaScriptCore/runtime/Arguments.h:232
> >          Register* registerArray = new Register[registerArraySize];
> >          memcpy(registerArray, d->registers - registerOffset, registerArraySize * sizeof(Register));
> > -        d->registerArray.set(registerArray);
> > +        d->registerArray = registerArray;
> >          d->registers = registerArray + registerOffset;
> 
> Needs an adoptArrayPtr. Probably won’t compile. I think this is may be why EWS bots are failing to build.

Sure enough.  I don't understand why XCode didn't complain about this.  Will fix.

[Ryosuke Niwa]

Created attachment 81445 [details]
fixed per Darin's comment

[Darin Adler]

Comment on attachment 81445 [details]
fixed per Darin's comment

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

> Source/JavaScriptCore/runtime/JSVariableObject.h:165
>      inline void JSVariableObject::setRegisters(Register* registers, Register* registerArray)
>      {
>          ASSERT(registerArray != d->registerArray.get());
> -        d->registerArray.set(registerArray);
> +        d->registerArray = adoptArrayPtr(registerArray);
>          d->registers = registers;
>      }

This is OK for landing, but really we need the adoptArrayPtr to be close to the actual allocation. In this case, setRegisters should be taking a PassOwnArrayPtr for registerArray. And JSGlobalObject::copyGlobalsFrom should be getting the register array in an OwnArrayPtr by calling JSVariableObject::copyRegisterArray. And inside copyRegisterArray, the adoptArrayPtr should be right there where it calls new.

[Ryosuke Niwa]

(In reply to comment #32)
> (From update of attachment 81445 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81445&action=review
> This is OK for landing, but really we need the adoptArrayPtr to be close to the actual allocation. In this case, setRegisters should be taking a PassOwnArrayPtr for registerArray. And JSGlobalObject::copyGlobalsFrom should be getting the register array in an OwnArrayPtr by calling JSVariableObject::copyRegisterArray. And inside copyRegisterArray, the adoptArrayPtr should be right there where it calls new.

I totally agree.  The only reason I didn't do was because this patch was already 47KB and adding that change seemed to bloat the patch.

[Ryosuke Niwa]

Committed r77785: <http://trac.webkit.org/changeset/77785>

[Early Warning System Bot]

Attachment 81445 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7701754

[WebKit Review Bot]

http://trac.webkit.org/changeset/77785 might have broken Chromium Win Release