Bug 30476 - [Qt] [Symbian] Set the capability and memory required to run QtWebKit for Symbian
Summary: [Qt] [Symbian] Set the capability and memory required to run QtWebKit for Sym...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC S60 3rd edition
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks: 27065
  Show dependency treegraph
 
Reported: 2009-10-16 23:39 PDT by Laszlo Gombos
Modified: 2009-10-24 08:28 PDT (History)
3 users (show)

See Also:


Attachments
1st try. (1.18 KB, patch)
2009-10-16 23:44 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff
proposed patch (1.59 KB, patch)
2009-10-18 19:58 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff
proposed patch (1.38 KB, patch)
2009-10-18 20:01 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff
3rd try (9.51 KB, patch)
2009-10-19 10:58 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Laszlo Gombos 2009-10-16 23:39:14 PDT
It seems that by default a Qt app has no Symbian capability assigned, which means for example that QtLauncher has no access to networking.
In addition QtWebkit needs more memory than the default Qt Symbian app.
Comment 1 Laszlo Gombos 2009-10-16 23:44:50 PDT
Created attachment 41353 [details]
1st try.

Assign all Symbian app capabilities except the Location capability.

Set the heap size to the 128kB - 32MB range.
Comment 2 Janne Koskinen 2009-10-17 02:07:40 PDT
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.
Comment 3 Norbert Leser 2009-10-18 16:29:18 PDT
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.
Comment 4 Laszlo Gombos 2009-10-18 19:46:55 PDT
(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.
Comment 5 Laszlo Gombos 2009-10-18 19:58:48 PDT
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.
Comment 6 Laszlo Gombos 2009-10-18 20:01:03 PDT
Created attachment 41391 [details]
proposed patch

Previous patch contains and extra line - remove it; sorry.
Comment 7 Norbert Leser 2009-10-18 20:09:27 PDT
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).
Comment 8 Janne Koskinen 2009-10-19 00:39:36 PDT
(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.
Comment 9 Laszlo Gombos 2009-10-19 10:58:44 PDT
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 10 Holger Freyther 2009-10-23 21:16:50 PDT
Comment on attachment 41430 [details]
3rd try

I see no harm with that.
Comment 11 WebKit Commit Bot 2009-10-24 08:28:28 PDT
Comment on attachment 41430 [details]
3rd try

Clearing flags on attachment: 41430

Committed r50026: <http://trac.webkit.org/changeset/50026>
Comment 12 WebKit Commit Bot 2009-10-24 08:28:33 PDT
All reviewed patches have been landed.  Closing bug.