Bug 60681 – Enable OwnPtr strict mode in PluginHalter

• Component: New Bugs
• Status: RESOLVED FIXED

[Patrick R. Gansterer]

Enable OwnPtr strict mode in PluginHalter

[Patrick R. Gansterer]

Created attachment 93229 [details]
Patch

[Adam Barth]

Comment on attachment 93229 [details]
Patch

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

I'm worried that this is going to create leaks.  I don't think we fully understand how memory is managed for the other clients and whether this client is different for a reason.

> Source/WebCore/ChangeLog:9
> +        PluginHalterClien gets allocated in WebKit code, so deleting it in WebCore is wrong.

PluginHalterClien => PluginHalterClient

[Patrick R. Gansterer]

Created attachment 93230 [details]
Patch

[Adam Barth]

Comment on attachment 93230 [details]
Patch

Thanks for fixing the typo, but there's still the question of whether this patch is creating a memory leak.

[Darin Adler]

Comment on attachment 93230 [details]
Patch

This patch removes a call to delete. Were we having problems with double deletion before? If not, why not?

[Patrick R. Gansterer]

(In reply to comment #4)
> Thanks for fixing the typo, but there's still the question of whether this patch is creating a memory leak.
I would say yes :-) But deleting in WebCore is wrong when we allocate in WebKit.

(In reply to comment #5)
> This patch removes a call to delete. Were we having problems with double deletion before? If not, why not?
Do you know where we delete the other members of PageClient?
AFAIKS they leak all at the moment. So PluginHalterClient should leak too, when the WebKit layer doesn't delete it.

[Adam Barth]

> AFAIKS they leak all at the moment. So PluginHalterClient should leak too, when the WebKit layer doesn't delete it.

That makes it sound like we should fix those leaks first rather than creating more.

[Patrick R. Gansterer]

(In reply to comment #7)
> > AFAIKS they leak all at the moment. So PluginHalterClient should leak too, when the WebKit layer doesn't delete it.
> 
> That makes it sound like we should fix those leaks first rather than creating more.

Of course! I'm already looking at the other classes, if they get deleted on mac....

[Patrick R. Gansterer]

Created attachment 93281 [details]
Patch

I now found the place of deletion. There are some uncool delete this statements.

[Early Warning System Bot]

Comment on attachment 93281 [details]
Patch

Attachment 93281 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8684769

[Patrick R. Gansterer]

Created attachment 93284 [details]
Patch

[Early Warning System Bot]

Comment on attachment 93284 [details]
Patch

Attachment 93284 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8688718

[WebKit Review Bot]

Comment on attachment 93284 [details]
Patch

Attachment 93284 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8691582

[WebKit Review Bot]

Comment on attachment 93284 [details]
Patch

Attachment 93284 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8689701

[Patrick R. Gansterer]

Created attachment 93286 [details]
Patch

[WebKit Review Bot]

Comment on attachment 93284 [details]
Patch

Attachment 93284 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8686759

[WebKit Review Bot]

Comment on attachment 93281 [details]
Patch

Attachment 93281 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8686762

[WebKit Review Bot]

Comment on attachment 93281 [details]
Patch

Attachment 93281 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8686764

[Darin Adler]

(In reply to comment #6)
> (In reply to comment #4)
> > Thanks for fixing the typo, but there's still the question of whether this patch is creating a memory leak.
> I would say yes :-) But deleting in WebCore is wrong when we allocate in WebKit.

Just for the record, I think you are overstating this rule. We can indeed have APIs that take allocated objects, take ownership of them, and then delete them. These objects can be created in WebKit code and then deleted in WebCore code. That’s one thing PassOwnPtr is good at -- making it explicit that there is a passing of ownership.

It’s OK to change the design here, but it would also be OK to make this client one that the WebCore objects takes ownership of.

[Adam Barth]

Comment on attachment 93286 [details]
Patch

Patrick, if you wouldn't mind re-doing this patch one more time, I think keeping the current ownership model would be better than adding more "delete this" calls.  Can we change the constructor to take a PassOwnPtr to document that it is taking ownership of the client?

[Patrick R. Gansterer]

(In reply to comment #20)
> (From update of attachment 93286 [details])
> Patrick, if you wouldn't mind re-doing this patch one more time, I think keeping the current ownership model would be better than adding more "delete this" calls.  Can we change the constructor to take a PassOwnPtr to document that it is taking ownership of the client?

I don't have a problem with doing this patch 3 times, if there is a reason for doing it. Making "perfect" code is a thing a like on WebKit :-)

So What's the correct behaviour now?
To be consistent we should
a) add a pluginHandlerDeleted() with "delete this" or
b) make the other clients deleted by WebCore too.
In case (b) PageClients should have OwnPtr members instead of the raw pointers, correct?

(In reply to comment #19)
> Just for the record, I think you are overstating this rule. We can indeed have APIs that take allocated objects, take ownership of them, and then delete them. These objects can be created in WebKit code and then deleted in WebCore code. That’s one thing PassOwnPtr is good at -- making it explicit that there is a passing of ownership.
IMHO that's a kind of layering violation. Even if it's not the case here, when two libraries use different memory allocation funtions, a new in WebKit with memory management A, can't be deleted in WebCore when it's using memory management B. I have some bad experience with this kind of problems, so i might be oversensitive on this topic.

[Darin Adler]

Client is a big category that is not entirely homogenous. For many clients we allow WebKit to do whatever lifetime management they like, and so the owner of the client inside WebCore cannot delete it; the end of life action may not even be deletion.

This client doesn’t have to be in that category. Or it could be changed. I’m not sure which would be better.

The design where the client holds an OwnPtr is less flexible, but far more foolproof than a design where the client calls delete explicitly.

I don’t think that a design change like this should be done in a bug titled “enable strict mode”.

[Darin Adler]

(In reply to comment #21)
> (In reply to comment #19)
> > Just for the record, I think you are overstating this rule. We can indeed have APIs that take allocated objects, take ownership of them, and then delete them. These objects can be created in WebKit code and then deleted in WebCore code. That’s one thing PassOwnPtr is good at -- making it explicit that there is a passing of ownership.
> IMHO that's a kind of layering violation. Even if it's not the case here, when two libraries use different memory allocation funtions, a new in WebKit with memory management A, can't be deleted in WebCore when it's using memory management B. I have some bad experience with this kind of problems, so i might be oversensitive on this topic.

I’m aware of this issue in other contexts. But it’s simply not an issue for the interface between WebCore and WebKit. Given how we do reference counting, it’s quite common for an object to be deleted by WebCore even if it was allocated by WebKit. Any reference-counted base class in WebCore that has a class in WebKit that’s derived from it falls into that category. So designing to avoid this issue elsewhere doesn’t provide significant value.

[Patrick R. Gansterer]

(In reply to comment #22)
> Client is a big category that is not entirely homogenous. For many clients we allow WebKit to do whatever lifetime management they like, and so the owner of the client inside WebCore cannot delete it; the end of life action may not even be deletion.

I don't like the difference between the clients.

> This client doesn’t have to be in that category. Or it could be changed. I’m not sure which would be better.

My first idea was to make PluginHalterClient behave like the other clients. I'm not a fan of delete this, but it's more flexible...

> I don’t think that a design change like this should be done in a bug titled “enable strict mode”.

Yes, but the bug was orginally only to fix the strict mode ;-)

So what should/can I do now? Should I only change the PluginHalterClient to PassOwnPtr?? (seams a little strange to me) I still prefere the delete this solution (because it's consistent in the current design) and move the ownership decision to a new bug.

[Darin Adler]

(In reply to comment #24)
> (In reply to comment #22)
> > Client is a big category that is not entirely homogenous. For many clients we allow WebKit to do whatever lifetime management they like, and so the owner of the client inside WebCore cannot delete it; the end of life action may not even be deletion.
> 
> I don't like the difference between the clients.

I believe not all clients are the same. Changing this one won’t make them all consistent.

> > This client doesn’t have to be in that category. Or it could be changed. I’m not sure which would be better.
> 
> My first idea was to make PluginHalterClient behave like the other clients. I'm not a fan of delete this, but it's more flexible.

Yes, more flexible is the benefit, and easier to use wrong and create a storage leak or lifetime bug is the cost. The point of OwnPtr is to make lifetime mistakes harder. I don’t think the extra flexibility here is worth it.

> > I don’t think that a design change like this should be done in a bug titled “enable strict mode”.
> 
> Yes, but the bug was orginally only to fix the strict mode ;-)
> 
> So what should/can I do now? Should I only change the PluginHalterClient to PassOwnPtr?

Yes. You can change its design later if we get consensus that we should; I think that the current design is safer and should not be changed to the more flexible but easier to get wrong design that we use for other clients, despite the fact that the class has the name “client”.

[Patrick R. Gansterer]

Created attachment 93329 [details]
Patch

[Adam Barth]

Comment on attachment 93329 [details]
Patch

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

Thanks!

> Source/WebCore/WebCore.exp.in:706
> -__ZN7WebCore4PageC1ERKNS0_11PageClientsE
> +__ZN7WebCore4PageC1ERNS0_11PageClientsE

There's probably a similar change for the Windows export list.

[Patrick R. Gansterer]

(In reply to comment #27)
> (From update of attachment 93329 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93329&action=review
> 
> Thanks!
> 
> > Source/WebCore/WebCore.exp.in:706
> > -__ZN7WebCore4PageC1ERKNS0_11PageClientsE
> > +__ZN7WebCore4PageC1ERNS0_11PageClientsE
> 
> There's probably a similar change for the Windows export list.

I'm waiting for the EWS to get the correct mangeled name.

[Patrick R. Gansterer]

Comment on attachment 93329 [details]
Patch

(In reply to comment #27) 
> Thanks!
NP!

> There's probably a similar change for the Windows export list.
Seams like no change is required.

[WebKit Commit Bot]

Comment on attachment 93329 [details]
Patch

Rejecting attachment 93329 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'land-a..." exit_code: 1

Last 500 characters of output:
autoinstalled/mechanize/_urllib2_fork.py", line 332, in _call_chain
    result = func(*args)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open
    return self.do_open(conn_factory, req)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open
    raise URLError(err)
urllib2.URLError: <urlopen error [Errno 60] Operation timed out>

Full output: http://queues.webkit.org/results/8685890

[Adam Barth]

Comment on attachment 93329 [details]
Patch

Interesting.  We've been hitting that error more recently for some reason.

[WebKit Commit Bot]

Comment on attachment 93329 [details]
Patch

Clearing flags on attachment: 93329

Committed r86391: <http://trac.webkit.org/changeset/86391>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

(In reply to comment #32)
> (From update of attachment 93329 [details])
> Clearing flags on attachment: 93329
> 
> Committed r86391: <http://trac.webkit.org/changeset/86391>

This change causes more than 20 tests to crash on both the Leopard Intel Debug Tests and Windows XP Debug Tests bot.

For Leopard Intel Debug Tests:

stdio: <http://build.webkit.org/builders/Leopard%20Intel%20Debug%20%28Tests%29/builds/30190/steps/layout-test/logs/stdio>
Results: <http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r86391%20(30190)/results.html>

For Windows XP Debug Tests:

stdio: <http://build.webkit.org/builders/Windows%20XP%20Debug%20%28Tests%29/builds/28600/steps/layout-test/logs/stdio>
Results: <http://build.webkit.org/results/Windows%20XP%20Debug%20(Tests)/r86391%20(28600)/results.html>

[Daniel Bates]

Committed build fix in changeset 86404 <http://trac.webkit.org/changeset/86404>.

[Ademar Reis]

Revision r86391 cherry-picked into qtwebkit-2.2 with commit 50cb4be <http://gitorious.org/webkit/qtwebkit/commit/50cb4be>
Revision r86404 cherry-picked into qtwebkit-2.2 with commit 1c63892 <http://gitorious.org/webkit/qtwebkit/commit/1c63892>