Bug 107397 – [Efl][WebGL] Add better support to track and free XResources.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kalyan]

We leak Memory during config selection as we dont free temp allocated visuals, config etc. It would be better to have some thing like a scoped class, this would free memory when class goes out of scope or when assigned a different value.

[Kalyan]

Created attachment 183669 [details]
patch

[Kalyan]

Created attachment 183674 [details]
patchv2

removed usused member varibles in X11WindowResources

[Laszlo Gombos]

Comment on attachment 183674 [details]
patchv2

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

> Source/WebCore/ChangeLog:8
> +        Covered by existing WebGL tests.

This is probably true but WebGL tests are not enabled for EFL on the bot so I think it is about time to hold on with re-factoring like this and enable the WebGL tests first.

[Kalyan]

(In reply to comment #3)
> (From update of attachment 183674 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183674&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Covered by existing WebGL tests.
> 
> This is probably true but WebGL tests are not enabled for EFL on the bot so I think it is about time to hold on with re-factoring like this and enable the WebGL tests first.

I do agree with you but I don't see this as refactoring but rather part of effort in fixing things to enable the layout tests in our port. I was atleast having constant WebProcess crashes(doing double deletions sometimes) without these changes.

[Kalyan]

(In reply to comment #3)
> (From update of attachment 183674 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183674&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Covered by existing WebGL tests.
> 
> This is probably true but WebGL tests are not enabled for EFL on the bot so I think it is about time to hold on with re-factoring like this and enable the WebGL tests first.

On side note we need 106878,107397,107178 fixed before trying to enable WebGL tests (atleast ones under fast/canvas/webgl). Atleast, I needed these before I could run the tests without any random crashes.

I will create a separate issue to enable to the tests.

[Kalyan]

Created attachment 183703 [details]
patchv3

updated the changelog

[Kalyan]

Created attachment 183837 [details]
patchv4

rebased patch. Not marking it for review yet. Layout tests with latest source seems stable even without this patch and don't see any de-allocation issues. The patch only fixes memory leaks and thus could be considered after enabling them.

[Noam Rosenthal]

Comment on attachment 183837 [details]
patchv4

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

> Source/WebCore/platform/graphics/surfaces/glx/X11WindowResources.h:49
> +struct ScopedXResource {

This is not needed. You can have OwnPtr with a specialized template, like GTK/Cairo does.

[Kalyan]

Created attachment 184862 [details]
patchv5

Changed name of ScopedXResource to ScopedPtrX11 and moved it to a separate header. Cannot use OwnPtr here with GLXFBConfig, as it is a pointer. 
i.e it is defined as typedef struct __GLXFBConfigRec *GLXFBConfig. One way would have been to handle this case separately but as we would need this change (ScopedPtrX11) for it, so I let it manage all the X Resources.

[Kalyan]

Created attachment 185079 [details]
patchv6

[Noam Rosenthal]

Comment on attachment 185079 [details]
patchv6

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

> Source/WebCore/ChangeLog:8
> +        We leak Memory during config selection as we dont free temp

temp -> temporary

> Source/WebCore/ChangeLog:35
> +        Calls XFree on pointer to memory allocated by X when the class goes out of scope.

Add some short explanation on why this was needed rather than using OwnPtr.

[Kalyan]

Created attachment 185694 [details]
patchforlanding

[Kalyan]

Created attachment 185698 [details]
patchforlanding

[EFL EWS Bot]

Comment on attachment 185698 [details]
patchforlanding

Attachment 185698 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16264129

[Kalyan]

Created attachment 185731 [details]
Patch

[WebKit Review Bot]

Comment on attachment 185731 [details]
Patch

Clearing flags on attachment: 185731

Committed r141404: <http://trac.webkit.org/changeset/141404>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.