Bug 153055 - Reduce PassRefPtr uses in dom - 3
Summary: Reduce PassRefPtr uses in dom - 3
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks: 144092
  Show dependency treegraph
 
Reported: 2016-01-12 21:08 PST by Gyuyoung Kim
Modified: 2016-01-18 18:56 PST (History)
8 users (show)

See Also:


Attachments
Patch (18.27 KB, patch)
2016-01-12 21:09 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (44.33 KB, application/zip)
2016-01-12 22:23 PST, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-yosemite (50.33 KB, application/zip)
2016-01-12 22:34 PST, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (51.22 KB, application/zip)
2016-01-12 22:34 PST, Build Bot
no flags Details
Patch (18.30 KB, patch)
2016-01-13 19:39 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch for landing (18.31 KB, patch)
2016-01-18 17:09 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2016-01-12 21:08:48 PST
SSIA
Comment 1 Gyuyoung Kim 2016-01-12 21:09:38 PST
Created attachment 268853 [details]
Patch
Comment 2 Build Bot 2016-01-12 22:23:12 PST
Comment on attachment 268853 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 3 Build Bot 2016-01-12 22:23:15 PST
Created attachment 268857 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 4 Build Bot 2016-01-12 22:34:07 PST
Comment on attachment 268853 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 5 Build Bot 2016-01-12 22:34:10 PST
Created attachment 268858 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 6 Build Bot 2016-01-12 22:34:43 PST
Comment on attachment 268853 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 7 Build Bot 2016-01-12 22:34:46 PST
Created attachment 268859 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 8 Gyuyoung Kim 2016-01-13 19:39:32 PST
Created attachment 268930 [details]
Patch
Comment 9 Alex Christensen 2016-01-14 21:29:25 PST
Comment on attachment 268930 [details]
Patch

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

> Source/WebCore/dom/MutationObserverInterestGroup.cpp:81
> -                mutationWithNullOldValue = MutationRecord::createWithNullOldValue(mutation).get();
> +                mutationWithNullOldValue = MutationRecord::createWithNullOldValue(*mutation).ptr();

This is not more unsafe than the old code.  I assume prpMutation will become a Ref<MutationRecord> instead of a PassRefPtr<MutationRecord>.  Why not do that change, too, in this patch?  The we wouldn't be adding another unchecked null dereference.
Comment 10 Gyuyoung Kim 2016-01-14 23:13:34 PST
(In reply to comment #9)
> Comment on attachment 268930 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268930&action=review
> 
> > Source/WebCore/dom/MutationObserverInterestGroup.cpp:81
> > -                mutationWithNullOldValue = MutationRecord::createWithNullOldValue(mutation).get();
> > +                mutationWithNullOldValue = MutationRecord::createWithNullOldValue(*mutation).ptr();
> 
> This is not more unsafe than the old code.  I assume prpMutation will become
> a Ref<MutationRecord> instead of a PassRefPtr<MutationRecord>.  Why not do
> that change, too, in this patch?  The we wouldn't be adding another
> unchecked null dereference.

When I tried to change it, patch is bigger than now. Because big change is able to sometime cause unexpected crash, I postponed it to next patch. However it is cool to change it in this patch together, let me try it.
Comment 11 Darin Adler 2016-01-18 14:24:10 PST
Comment on attachment 268930 [details]
Patch

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

> Source/WebCore/dom/ShadowRoot.h:84
> +    RefPtr<Node> cloneNode(bool, ExceptionCode&);

After studying the code, I realize that this function is dead code, never called. The right thing to do in this patch is to delete this function entirely.

Separately, someone needs to make a test case that tries to clone a shadow root. I expect they will find that no exception is raised, and we will need code changes to make that happen. But this function won’t do the trick.
Comment 12 Gyuyoung Kim 2016-01-18 17:09:03 PST
Created attachment 269247 [details]
Patch for landing
Comment 13 Gyuyoung Kim 2016-01-18 18:20:26 PST
(In reply to comment #11)
> Comment on attachment 268930 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268930&action=review
> 
> > Source/WebCore/dom/ShadowRoot.h:84
> > +    RefPtr<Node> cloneNode(bool, ExceptionCode&);
> 
> After studying the code, I realize that this function is dead code, never
> called. The right thing to do in this patch is to delete this function
> entirely.

Thanks, removed.
Comment 14 WebKit Commit Bot 2016-01-18 18:56:47 PST
Comment on attachment 269247 [details]
Patch for landing

Clearing flags on attachment: 269247

Committed r195243: <http://trac.webkit.org/changeset/195243>
Comment 15 WebKit Commit Bot 2016-01-18 18:56:54 PST
All reviewed patches have been landed.  Closing bug.