Introduce Range::dispose() for prompt detachment from owner Document.

Introduce Range::dispose() for prompt detachment from owner Document.

With Oilpan, Range objects become detached from their Document once GC
determines that the Range object is no longer referred to and can be
swept out, along with clearing out the object from the Document's weak
map. If GCs aren't otherwise being triggered regularly, this can in
some cases lead to unnecessary buildup of weakly held, but effectively
dead objects in that Document map. Something which slows down GC once
it eventually strikes.

To address, we introduce a dispose() method over Range so as to let code
handle the cases where it is known that the Range object is no longer
referenced & used and can be promptly detached from its Document. Less
GC overhead being the (desired) result.

R=yosin,haraken
BUG=388681
Committed: https://crrev.com/da665676d70ea64c87a8ef8d8b4b9c4d933397f5
git-svn-id: svn://svn.chromium.org/blink/trunk@202138 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[sof, 2015-09-10 12:48:01]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org, yosin@chromium.org

[sof, 2015-09-10 12:48:01]

please take a look.

if the preference is to re-purpose detach() rather than introduce dispose(),
then that shouldn't be a problem.

[yosin_UTC9, 2015-09-11 01:22:27]

>if the preference is to re-purpose detach() rather than 
> introduce dispose(), then that shouldn't be a problem.

I'm fine with introducing |dispose()| protocol into Blink.

My concern is Range::detach() is no-op now and future by
DOM4 spec.

It is easy to make applications, especially for marketing
benchmark, which use lots of Range
objects to make Blink slow down but other browsers not.

https://codereview.chromium.org/1335573004/diff/1/Source/web/WebLocalFrameImp...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1335573004/diff/1/Source/web/WebLocalFrameImp...
Source/web/WebLocalFrameImpl.cpp:262:
range->selectNodeContents(document->body(), exceptionState);
We can use |EphemeralRange| here.

// ClusterFuzz can changes document w/o body.
if (!document->body())
    return;
const EphemeralRange range = rangeOfContents(*document->body());

[sof, 2015-09-11 06:27:09]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-11 06:27:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1335573004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1335573004/20001

[sof, 2015-09-11 06:32:12]

yosin writes:
> I'm fine with introducing |dispose()| protocol into Blink.
> 
> My concern is Range::detach() is no-op now and future by DOM4 spec.

I see, we're not actually allowed per spec to have detach() be anything other
than a no-op,

 https://dom.spec.whatwg.org/#dom-range-detach

No problem rewiring the IDL so as to have the exposed Range.detach() invoke some
other stub method, but perhaps that would just be too confusing. I will assume
so.

https://codereview.chromium.org/1335573004/diff/1/Source/web/WebLocalFrameImp...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1335573004/diff/1/Source/web/WebLocalFrameImp...
Source/web/WebLocalFrameImpl.cpp:262:
range->selectNodeContents(document->body(), exceptionState);
On 2015/09/11 01:22:27, Yosi_UTC9 wrote:
> We can use |EphemeralRange| here.
> 
> // ClusterFuzz can changes document w/o body.
> if (!document->body())
>     return;
> const EphemeralRange range = rangeOfContents(*document->body());

Nice; done, I think.

[yosin_UTC9, 2015-09-11 07:07:19]

So, WebLocalFrameImpl.cpp doesn't use Range::dispose().

https://codereview.chromium.org/1335573004/diff/20001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1335573004/diff/20001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:258: RefPtrWillBeRawPtr<Range>
range(document->createRange());
Thus, we don't need to use |Range|.
We may want to move this change in another CL.

[sof, 2015-09-11 07:11:43]

https://codereview.chromium.org/1335573004/diff/20001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1335573004/diff/20001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:258: RefPtrWillBeRawPtr<Range>
range(document->createRange());
On 2015/09/11 07:07:19, Yosi_UTC9 wrote:
> Thus, we don't need to use |Range|.
> We may want to move this change in another CL.

I'm sorry, spaced out completely - we clearly don't need this Range allocation.
Now g/done.

[sof, 2015-09-11 08:49:21]

this now looks ready to go.. (before the weekend :) ) ?

[yosin_UTC9, 2015-09-11 13:07:43]

lgtm

Please wait for core/dom and web/ OWNERS.
I recommend to move WebLocalFrame.cpp changes to another CL,
since it isn't relate to introducing Range::dispose().

[sof, 2015-09-11 13:29:49]

haraken@: owner ptal?

(i can certainly split out and separately expedite, if wanted.)

[haraken, 2015-09-11 13:30:27]

core/ and web/ LGTM

[sof, 2015-09-11 13:44:11]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-09-11 13:44:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/1335573004/#ps60001
(title: "split out rangeContents() switch")

[commit-bot: I haz the power, 2015-09-11 13:44:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1335573004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1335573004/60001

[commit-bot: I haz the power, 2015-09-11 15:10:25]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202138

[commit-bot: I haz the power, 2015-09-23 12:22:29]

Patchset 4 (id:??) landed as
https://crrev.com/da665676d70ea64c87a8ef8d8b4b9c4d933397f5