Bug 57877 – [Qt][WK2][Symbian] Remove use of stack arrays with variable size

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Siddharth Mathur]

RVCT 4.0 behaves badly when stack arrays are declared with unknown-at-build-time number of elements. 

E.g char myBuffer[object.foo()]; 

We should simply use new and delete for such arrays to help with compiler portability

[Siddharth Mathur]

Created attachment 88306 [details]
Patch with RVCT 4.0 and/or Symbian fixes

[Siddharth Mathur]

Comment on attachment 88306 [details]
Patch with RVCT 4.0 and/or Symbian fixes

The "MessageBodyIsOOL = 1U << 31" change is to fix RVCT 4.0 compiler warnings about attempting to do bit-shift operations on a signed int.

[Balazs Kelemen]

Comment on attachment 88306 [details]
Patch with RVCT 4.0 and/or Symbian fixes

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

Informal review.

Avoid non-standard C++ is definitely a good idea but there are some problems.

> Source/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:232
> +            
> +            delete[] fileDescriptors;
> +            delete[] attachmentSizes;

Normally we use OwnPtr and OwnArrayPtr for temporary heap allocated memory. Those clean up automatically.

> Source/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:367
> +    
> +    delete[] attachmentFDBuffer;
> +    delete[] attachmentSizes;
> +

Ditto.

> Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:100
> +#if !OS(SYMBIAN)
> +    if (socketpair(AF_UNIX, SOCK_DGRAM, 0, sockets) == -1) 
> +#endif
> +    {

I'm confused by this change. This is the Symbian fix, right? Maybe it should have it's own patch.
Anyway, is it good to make the ASSERT_NOT_REACHED() unconditionally on Symbian?

[Benjamin Poulain]

(In reply to comment #0)
> RVCT 4.0 behaves badly when stack arrays are declared with unknown-at-build-time number of elements. 
> 
> E.g char myBuffer[object.foo()]; 

Could you explain what happens in this case with RVCT 4?

Allocating dynamic arrays on the heap can have serious performance impact (not in this case ofc). Allocating everything on the heap seems ridiculous without a bit more context.

[Siddharth Mathur]

(In reply to comment #4)
> (In reply to comment #0)
> > RVCT 4.0 behaves badly when stack arrays are declared with unknown-at-build-time number of elements. 
> > 
> > E.g char myBuffer[object.foo()]; 
> 
> Could you explain what happens in this case with RVCT 4?
> 

Here are the errors which result during RVCT 4.0 link stage. They appear to happen because the compiler cannot detect the size of the arrays at compile time, and hence inserts references to malloc() and free() (with the wrong visibility?) in the generated code. 

Error: L6410E: Symbol free with non STV_DEFAULT visibility STV_HIDDEN should be resolved statically, cannot use definition in libc{00010001}.dso.
Error: L6410E: Symbol malloc with non STV_DEFAULT visibility STV_HIDDEN should be resolved statically, cannot use definition in libc{00010001}.dso.

I used the "fromelf" tool to inspect which source file the unexpected visibility change in malloc and free was coming from, and it turned out to be ConnectionQt.cpp. No other file in WebKit is affected by this issue.  
http://docs.huihoo.com/symbian/nokia-symbian3-developers-library-v0.8/GUID-0E3E2FAD-FC85-5995-8B5C-8F1C1A4186D0.html

[Balazs Kelemen]

> No other file in WebKit is affected by this issue.

Because generally we do not use variable size arrays (or any other non-standard construction as I know). I think heap allocation is the correct solution here.

[Siddharth Mathur]

Created attachment 88510 [details]
updated patch. 

Use scoped array (OwnArrayPtr) as suggested. 

Removed the socketpair change for now. Perhaps I will make a different bug? 
I would like to get CONFIG+=webkit2 to compile+link correctly on the buildbot before my pure Symbian IPC works starts landing, so as to get a baseline.

[Benjamin Poulain]

Comment on attachment 88510 [details]
updated patch. 

Look reasonable. With the smart pointer it is not too ugly.

No tests? ;)

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 88510 [details]:

animations/suspend-resume-animation.html bug 48161 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 88510 [details]
updated patch. 

Clearing flags on attachment: 88510

Committed r83116: <http://trac.webkit.org/changeset/83116>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Siddharth Mathur]

Created attachment 88647 [details]
Patch to fix Symbian Wk2 build until pure Symbian IPC and process launching is done

Please let me know if I should open a new bug. Since the discussion was here, I took the lazy route.

[Siddharth Mathur]

Reopening for now.

[Siddharth Mathur]

Created attachment 88649 [details]
Minor comment improvement (added "FIXME")

[Laszlo Gombos]

Comment on attachment 88649 [details]
Minor comment improvement (added "FIXME")

r=me.

[WebKit Commit Bot]

Comment on attachment 88649 [details]
Minor comment improvement (added "FIXME")

Clearing flags on attachment: 88649

Committed r83178: <http://trac.webkit.org/changeset/83178>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Siddharth Mathur]

Created attachment 88834 [details]
Rapply patch after r83215

[Siddharth Mathur]

r83215 accidently un-applied this patch. Resubmit

[Martin Robinson]

Comment on attachment 88834 [details]
Rapply patch after r83215

Thanks for resubmitting this. Sorry that I managed to lose it.

[WebKit Commit Bot]

Comment on attachment 88834 [details]
Rapply patch after r83215

Clearing flags on attachment: 88834

Committed r83323: <http://trac.webkit.org/changeset/83323>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.