Bug 60681

Summary: Enable OwnPtr strict mode in PluginHalter
Product: WebKit Reporter: Patrick R. Gansterer <paroga>
Component: New BugsAssignee: Patrick R. Gansterer <paroga>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ademar, commit-queue, darin, dbates, dglazkov, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
abarth: review-
Patch
webkit-ews: commit-queue-
Patch
webkit-ews: commit-queue-
Patch
none
Patch none

Description Patrick R. Gansterer 2011-05-11 18:05:48 PDT
Enable OwnPtr strict mode in PluginHalter
Comment 1 Patrick R. Gansterer 2011-05-11 18:16:14 PDT
Created attachment 93229 [details]
Patch
Comment 2 Adam Barth 2011-05-11 18:17:51 PDT
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
Comment 3 Patrick R. Gansterer 2011-05-11 18:20:51 PDT
Created attachment 93230 [details]
Patch
Comment 4 Adam Barth 2011-05-11 18:22:06 PDT
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.
Comment 5 Darin Adler 2011-05-11 18:23:16 PDT
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?
Comment 6 Patrick R. Gansterer 2011-05-11 18:25:55 PDT
(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.
Comment 7 Adam Barth 2011-05-11 18:27:10 PDT
> 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.
Comment 8 Patrick R. Gansterer 2011-05-11 18:29:39 PDT
(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....
Comment 9 Patrick R. Gansterer 2011-05-12 06:58:02 PDT
Created attachment 93281 [details]
Patch

I now found the place of deletion. There are some uncool delete this statements.
Comment 10 Early Warning System Bot 2011-05-12 07:09:01 PDT
Comment on attachment 93281 [details]
Patch

Attachment 93281 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8684769
Comment 11 Patrick R. Gansterer 2011-05-12 07:19:30 PDT
Created attachment 93284 [details]
Patch
Comment 12 Early Warning System Bot 2011-05-12 07:29:39 PDT
Comment on attachment 93284 [details]
Patch

Attachment 93284 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8688718
Comment 13 WebKit Review Bot 2011-05-12 07:30:14 PDT
Comment on attachment 93284 [details]
Patch

Attachment 93284 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8691582
Comment 14 WebKit Review Bot 2011-05-12 07:34:28 PDT
Comment on attachment 93284 [details]
Patch

Attachment 93284 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8689701
Comment 15 Patrick R. Gansterer 2011-05-12 07:39:35 PDT
Created attachment 93286 [details]
Patch
Comment 16 WebKit Review Bot 2011-05-12 07:41:53 PDT
Comment on attachment 93284 [details]
Patch

Attachment 93284 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8686759
Comment 17 WebKit Review Bot 2011-05-12 07:51:13 PDT
Comment on attachment 93281 [details]
Patch

Attachment 93281 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8686762
Comment 18 WebKit Review Bot 2011-05-12 07:59:09 PDT
Comment on attachment 93281 [details]
Patch

Attachment 93281 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8686764
Comment 19 Darin Adler 2011-05-12 09:27:09 PDT
(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.
Comment 20 Adam Barth 2011-05-12 10:24:15 PDT
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?
Comment 21 Patrick R. Gansterer 2011-05-12 11:14:53 PDT
(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.
Comment 22 Darin Adler 2011-05-12 11:28:24 PDT
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”.
Comment 23 Darin Adler 2011-05-12 11:30:17 PDT
(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.
Comment 24 Patrick R. Gansterer 2011-05-12 11:50:50 PDT
(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.
Comment 25 Darin Adler 2011-05-12 11:53:23 PDT
(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”.
Comment 26 Patrick R. Gansterer 2011-05-12 13:36:38 PDT
Created attachment 93329 [details]
Patch
Comment 27 Adam Barth 2011-05-12 13:43:38 PDT
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.
Comment 28 Patrick R. Gansterer 2011-05-12 13:44:44 PDT
(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.
Comment 29 Patrick R. Gansterer 2011-05-12 14:11:59 PDT
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.
Comment 30 WebKit Commit Bot 2011-05-12 15:37:35 PDT
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
Comment 31 Adam Barth 2011-05-12 15:43:14 PDT
Comment on attachment 93329 [details]
Patch

Interesting.  We've been hitting that error more recently for some reason.
Comment 32 WebKit Commit Bot 2011-05-12 16:06:28 PDT
Comment on attachment 93329 [details]
Patch

Clearing flags on attachment: 93329

Committed r86391: <http://trac.webkit.org/changeset/86391>
Comment 33 WebKit Commit Bot 2011-05-12 16:06:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 Daniel Bates 2011-05-12 17:28:55 PDT
(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>
Comment 35 Daniel Bates 2011-05-12 17:29:27 PDT
Committed build fix in changeset 86404 <http://trac.webkit.org/changeset/86404>.
Comment 36 Ademar Reis 2011-05-31 07:03:43 PDT
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>