Bug 30476

Created attachment 41353 [details] 1st try. Assign all Symbian app capabilities except the Location capability. Set the heap size to the 128kB - 32MB range.

I think CAP_APPLICATION is Nokia S60 specific MACRO and might not work on all Symbian development environments. Current required capabilities for using QtWebkit are ReadUserData,WriteUserData and NetworkServices. ReadUserData and WriteUserData for database access and NetworkServices for connection.

Thanks, for working on a cleanup. - Removing EPOCALLOWDLLDATA from all executables makes sense. - You are right, it appears that EPOCSTACKSIZE is set by qmake to the max value (0x14000) already, and it would be redundant. The problem though is that it may not be guaranteed that all qmake versions will set that value. If not set, the symbian's build system falls back to the default, which is way lower than needed. It might be safer to keep the definition in all exe's .pro files, even if redundant. - Janne is right that the basic capabilities needed are "ReadUserData WriteUserData NetworkServices". We should set these explicitly for all executables under WebKit/qt, instead of CAP_APPLICATION (I also saw that this macro is not available on public SDKs). - The problem with the previous one is that you set it in WebKit.pri, which is also processed by WebCore.pro. QtWebKit.dll, however, should have the max set of capabilities that calling processes may require. Per convention, for shared libraries that are used by a variety of apps, it is "All -Tcb". Otherwise, we run into the problem that apps which require certain capabilities outside the minimally needed ones cannot load the dll (the side-effect of the weird capabilities model in symbian). We could add the missing capabilities in WebCore.pro (I think), but that doesn't appear to be clean. Thus, I'd suggest to keep TARGET.CAPABILITY definitions in the respective .pro files of the executables (QtLauncher, QVGLauncher, all tst_*). - This patch only addresses WebKit.pri but I assume that your intention was to also cleanup the respective .pro files as well, once we have an understand what goes where.

(In reply to comment #3) > Thanks, for working on a cleanup. Thanks for the review. > > - Removing EPOCALLOWDLLDATA from all executables makes sense. > > - You are right, it appears that EPOCSTACKSIZE is set by qmake to the max value > (0x14000) already, and it would be redundant. The problem though is that it may > not be guaranteed that all qmake versions will set that value. If not set, the > symbian's build system falls back to the default, which is way lower than > needed. It might be safer to keep the definition in all exe's .pro files, even > if redundant. Ok, will add EPOCSTACKSIZE to the patch. > - Janne is right that the basic capabilities needed are "ReadUserData > WriteUserData NetworkServices". We should set these explicitly for all > executables under WebKit/qt, instead of CAP_APPLICATION (I also saw that this > macro is not available on public SDKs). Good catch on the public SDK - will fix the patch. > - The problem with the previous one is that you set it in WebKit.pri, which is > also processed by WebCore.pro. WebCore.pro does include WebKit.pri, but it will never reach the patched section. I admit it is hard to follow all the conditions in WebKit.pri (hint check out building-libs). The change only impacts executables (QtLauncher, QVGLauncher, all tst_*) and not QtWebKit.dll (you can test it by temporary adding an error() to the patched section of WebKit.pri and qmake WebCore.pro). > QtWebKit.dll, however, should have the max set > of capabilities that calling processes may require. Per convention, for shared > libraries that are used by a variety of apps, it is "All -Tcb". Otherwise, we > run into the problem that apps which require certain capabilities outside the > minimally needed ones cannot load the dll (the side-effect of the weird > capabilities model in symbian). We could add the missing capabilities in > WebCore.pro (I think), but that doesn't appear to be clean. Thus, I'd suggest > to keep TARGET.CAPABILITY definitions in the respective .pro files of the > executables (QtLauncher, QVGLauncher, all tst_*). > > - This patch only addresses WebKit.pri but I assume that your intention was to > also cleanup the respective .pro files as well, once we have an understand what > goes where. I think this change will set the correct capabilities/mem attributes for (QtLauncher, QVGLauncher, all tst_*) in one location.

Created attachment 41390 [details] proposed patch Thanks for Janne and Norbert for the comments - the patch addressing their input. Changes: - Use explicit Symbian capabilities instead of CAP_APPLICATION - and use only those capabilities that is actually needed. - Set the stack size to 80 Kb.

Created attachment 41391 [details] proposed patch Previous patch contains and extra line - remove it; sorry.

I understand that putting TARGET.CAPABILITY in WebKit.pri keeps it in one location instead of every single .pro for executables. Still, specifically since it it hard to parse where it actually applies, I believe that the .pro files for each exe is the right place for this sort of thing. It defeats security if it is not fully transparent to the user. Also, there might me certain executables in the future, such as specific tests, that may need to have different capabilities (e.f., tests for plugins that access local resources).

(In reply to comment #7) > I understand that putting TARGET.CAPABILITY in WebKit.pri keeps it in one > location instead of every single .pro for executables. Still, specifically > since it it hard to parse where it actually applies, I believe that the .pro > files for each exe is the right place for this sort of thing. It defeats > security if it is not fully transparent to the user. Also, there might me > certain executables in the future, such as specific tests, that may need to > have different capabilities (e.f., tests for plugins that access local > resources). I have to agree. Better place for capabilities is the application .pro file. target.epocheapsize and target.epocstacksize could be in the webkit.pri file. Although, I don't agree with setting the stacksize, can't see situation when qmake wouldn't set this.

Created attachment 41430 [details] 3rd try Place Symbian capabilities in the application .pro files (instead of WebKit.pri) - as it was suggested by Norbert and Janne.

Comment on attachment 41430 [details] 3rd try I see no harm with that.

Comment on attachment 41430 [details] 3rd try Clearing flags on attachment: 41430 Committed r50026: <http://trac.webkit.org/changeset/50026>

All reviewed patches have been landed. Closing bug.