Bug 52867 - OwnArraryPtr.h uses deleteOwnedPtr but doesn’t include OwnPtrCommon.h
Summary: OwnArraryPtr.h uses deleteOwnedPtr but doesn’t include OwnPtrCommon.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-20 21:22 PST by Ryosuke Niwa
Modified: 2011-02-06 21:25 PST (History)
14 users (show)

See Also:


Attachments
Include OwnPtrCommon.h in OwnArrayPtr.h (934 bytes, patch)
2011-01-20 21:53 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
get rid of set (25.37 KB, patch)
2011-01-21 22:17 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixed platform code (30.73 KB, patch)
2011-01-21 23:07 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
more port specific fixes (32.66 KB, patch)
2011-01-22 00:43 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
another attempt (40.17 KB, patch)
2011-01-22 03:40 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
yet another attempt (44.93 KB, patch)
2011-01-22 12:47 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixed v8 binding (44.93 KB, patch)
2011-01-22 17:10 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixed cr-mac build (46.69 KB, patch)
2011-02-06 18:06 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixed per Darin's comment (47.20 KB, patch)
2011-02-06 20:35 PST, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-01-20 21:22:37 PST
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.
Comment 1 Ryosuke Niwa 2011-01-20 21:53:14 PST
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.
Comment 2 Maciej Stachowiak 2011-01-20 22:01:55 PST
Comment on attachment 79693 [details]
Include OwnPtrCommon.h in OwnArrayPtr.h

r=me
Comment 3 Ryosuke Niwa 2011-01-20 22:04:18 PST
(In reply to comment #2)
> (From update of attachment 79693 [details])
> r=me

Thanks for the review!
Comment 4 Ryosuke Niwa 2011-01-20 22:10:51 PST
Comment on attachment 79693 [details]
Include OwnPtrCommon.h in OwnArrayPtr.h

Clearing flags on attachment: 79693

Committed r76334: <http://trac.webkit.org/changeset/76334>
Comment 5 Ryosuke Niwa 2011-01-20 22:10:57 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Darin Adler 2011-01-21 11:40:14 PST
This was the wrong fix. OwnArrayPtr<T>::set needs to use deletedOwnedArrayPtr. Or it needs to be removed.
Comment 7 Ryosuke Niwa 2011-01-21 11:49:51 PST
(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!
Comment 8 Darin Adler 2011-01-21 11:53:12 PST
(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.
Comment 9 Ryosuke Niwa 2011-01-21 17:14:37 PST
Let me do this over the weekend (hopefully).
Comment 10 Ryosuke Niwa 2011-01-21 22:17:08 PST
Created attachment 79827 [details]
get rid of set
Comment 11 WebKit Review Bot 2011-01-21 22:29:35 PST
Attachment 79827 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7521251
Comment 12 Build Bot 2011-01-21 22:46:15 PST
Attachment 79827 [details] did not build on win:
Build output: http://queues.webkit.org/results/7622260
Comment 13 Ryosuke Niwa 2011-01-21 23:07:39 PST
Created attachment 79830 [details]
fixed platform code
Comment 14 WebKit Review Bot 2011-01-21 23:22:28 PST
Attachment 79830 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7510268
Comment 15 Build Bot 2011-01-22 00:02:05 PST
Attachment 79830 [details] did not build on win:
Build output: http://queues.webkit.org/results/7596258
Comment 16 Ryosuke Niwa 2011-01-22 00:43:17 PST
Created attachment 79834 [details]
more port specific fixes
Comment 17 WebKit Review Bot 2011-01-22 00:57:27 PST
Attachment 79834 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7561293
Comment 18 Build Bot 2011-01-22 01:38:41 PST
Attachment 79834 [details] did not build on win:
Build output: http://queues.webkit.org/results/7513275
Comment 19 Ryosuke Niwa 2011-01-22 03:40:06 PST
Created attachment 79839 [details]
another attempt
Comment 20 WebKit Review Bot 2011-01-22 04:03:01 PST
Attachment 79839 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7556269
Comment 21 Build Bot 2011-01-22 04:35:15 PST
Attachment 79839 [details] did not build on win:
Build output: http://queues.webkit.org/results/7536279
Comment 22 Ryosuke Niwa 2011-01-22 12:47:47 PST
Created attachment 79854 [details]
yet another attempt
Comment 23 WebKit Review Bot 2011-01-22 13:10:47 PST
Attachment 79854 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7505249
Comment 24 Ryosuke Niwa 2011-01-22 17:10:01 PST
Created attachment 79861 [details]
fixed v8 binding
Comment 25 WebKit Review Bot 2011-01-22 17:48:37 PST
Attachment 79861 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7626241
Comment 26 Ryosuke Niwa 2011-02-06 18:06:47 PST
Created attachment 81438 [details]
fixed cr-mac build
Comment 27 Early Warning System Bot 2011-02-06 18:30:06 PST
Attachment 81438 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7700733
Comment 28 Ryosuke Niwa 2011-02-06 19:31:48 PST
(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.
Comment 29 Darin Adler 2011-02-06 20:04:40 PST
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.
Comment 30 Ryosuke Niwa 2011-02-06 20:17:01 PST
(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.
Comment 31 Ryosuke Niwa 2011-02-06 20:35:18 PST
Created attachment 81445 [details]
fixed per Darin's comment
Comment 32 Darin Adler 2011-02-06 20:40:31 PST
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.
Comment 33 Ryosuke Niwa 2011-02-06 20:43:57 PST
(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.
Comment 34 Ryosuke Niwa 2011-02-06 21:01:45 PST
Committed r77785: <http://trac.webkit.org/changeset/77785>
Comment 35 Early Warning System Bot 2011-02-06 21:15:34 PST
Attachment 81445 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7701754
Comment 36 WebKit Review Bot 2011-02-06 21:25:04 PST
http://trac.webkit.org/changeset/77785 might have broken Chromium Win Release