Bug 57877

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

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 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?

(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.

(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

> 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.

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 on attachment 88510 [details] updated patch. Look reasonable. With the smart pointer it is not too ugly. No tests? ;)

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 on attachment 88510 [details] updated patch. Clearing flags on attachment: 88510 Committed r83116: <http://trac.webkit.org/changeset/83116>

All reviewed patches have been landed. Closing bug.

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.

Reopening for now.

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

Comment on attachment 88649 [details] Minor comment improvement (added "FIXME") r=me.

Comment on attachment 88649 [details] Minor comment improvement (added "FIXME") Clearing flags on attachment: 88649 Committed r83178: <http://trac.webkit.org/changeset/83178>

All reviewed patches have been landed. Closing bug.

Created attachment 88834 [details] Rapply patch after r83215

r83215 accidently un-applied this patch. Resubmit

Comment on attachment 88834 [details] Rapply patch after r83215 Thanks for resubmitting this. Sorry that I managed to lose it.

Comment on attachment 88834 [details] Rapply patch after r83215 Clearing flags on attachment: 88834 Committed r83323: <http://trac.webkit.org/changeset/83323>

All reviewed patches have been landed. Closing bug.