Bug 57877

Summary: [Qt][WK2][Symbian] Remove use of stack arrays with variable size
Product: WebKit Reporter: Siddharth Mathur <s.mathur>
Component: WebKit2Assignee: Siddharth Mathur <s.mathur>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, kbalazs, laszlo.gombos
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: S60 Hardware   
OS: S60 3rd edition   
Attachments:
Description Flags
Patch with RVCT 4.0 and/or Symbian fixes
none
updated patch.
none
Patch to fix Symbian Wk2 build until pure Symbian IPC and process launching is done
none
Minor comment improvement (added "FIXME")
none
Rapply patch after r83215 none

Description Siddharth Mathur 2011-04-05 13:18:22 PDT
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
Comment 1 Siddharth Mathur 2011-04-05 14:03:34 PDT
Created attachment 88306 [details]
Patch with RVCT 4.0 and/or Symbian fixes
Comment 2 Siddharth Mathur 2011-04-05 14:05:24 PDT
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.
Comment 3 Balazs Kelemen 2011-04-05 15:22:37 PDT
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?
Comment 4 Benjamin Poulain 2011-04-06 04:41:51 PDT
(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.
Comment 5 Siddharth Mathur 2011-04-06 10:14:43 PDT
(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
Comment 6 Balazs Kelemen 2011-04-06 14:01:16 PDT
> 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.
Comment 7 Siddharth Mathur 2011-04-06 14:11:19 PDT
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.
Comment 8 Benjamin Poulain 2011-04-06 14:22:37 PDT
Comment on attachment 88510 [details]
updated patch. 

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

No tests? ;)
Comment 9 WebKit Commit Bot 2011-04-06 16:03:00 PDT
The commit-queue encountered the following flaky tests while processing attachment 88510 [details]:

animations/suspend-resume-animation.html bug 48161 (author: cmarrin@apple.com)
The commit-queue is continuing to process your patch.
Comment 10 WebKit Commit Bot 2011-04-06 16:06:23 PDT
Comment on attachment 88510 [details]
updated patch. 

Clearing flags on attachment: 88510

Committed r83116: <http://trac.webkit.org/changeset/83116>
Comment 11 WebKit Commit Bot 2011-04-06 16:06:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Siddharth Mathur 2011-04-07 08:48:34 PDT
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.
Comment 13 Siddharth Mathur 2011-04-07 08:49:55 PDT
Reopening for now.
Comment 14 Siddharth Mathur 2011-04-07 09:05:18 PDT
Created attachment 88649 [details]
Minor comment improvement (added "FIXME")
Comment 15 Laszlo Gombos 2011-04-07 09:12:28 PDT
Comment on attachment 88649 [details]
Minor comment improvement (added "FIXME")

r=me.
Comment 16 WebKit Commit Bot 2011-04-07 09:42:10 PDT
Comment on attachment 88649 [details]
Minor comment improvement (added "FIXME")

Clearing flags on attachment: 88649

Committed r83178: <http://trac.webkit.org/changeset/83178>
Comment 17 WebKit Commit Bot 2011-04-07 09:42:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Siddharth Mathur 2011-04-08 10:00:05 PDT
Created attachment 88834 [details]
Rapply patch after r83215
Comment 19 Siddharth Mathur 2011-04-08 10:01:05 PDT
r83215 accidently un-applied this patch. Resubmit
Comment 20 Martin Robinson 2011-04-08 10:05:29 PDT
Comment on attachment 88834 [details]
Rapply patch after r83215

Thanks for resubmitting this. Sorry that I managed to lose it.
Comment 21 WebKit Commit Bot 2011-04-08 12:30:33 PDT
Comment on attachment 88834 [details]
Rapply patch after r83215

Clearing flags on attachment: 88834

Committed r83323: <http://trac.webkit.org/changeset/83323>
Comment 22 WebKit Commit Bot 2011-04-08 12:30:38 PDT
All reviewed patches have been landed.  Closing bug.