Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(243)

Issue 311803003: [oilpan]: Avoid refcounting QualifiedName's nullQName when tracing. (Closed)

Created:
6 years, 6 months ago by wibling-chromium
Modified:
6 years, 6 months ago
CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, blink-reviews-dom_chromium.org, dglazkov+blink, krit, dstockwell, eae+blinkwatch, ed+blinkwatch_opera.com, Eric Willigers, f(malita), fs, gyuyoung.kim_webkit.org, kouhei+svg_chromium.org, Mike Lawther (Google), pdr., rjwright, rwlbuis, Stephen Chennney, shans, sof, Steve Block, Timothy Loh
Visibility:
Public.

Description

[oilpan]: Avoid refcounting QualifiedName's nullQName when tracing. When doing tracing across threads I hit an assert due to tracing the SMILTimeContainer's hashmap from a different thread than the one it was allocated on. Basically in HashTable's trace method we call isEmptyOrDeletedBucket which ends up calling PairHashTraits::emptyValue which makes a pair<WeakMember<SVGElement>, QualifiedName> initialized with the individual empty values. This causes the nullQName to be copied to the new pair which causes a ref() in RefCountedBase hitting ASSERT(m_verifier.isSafeToUse()). With this change all global or static local QualifiedNames are ignoring ref counting. This is similar to the way StringImpl works. R=ager@chromium.org, erik.corry@gmail.com, haraken@chromium.org, oilpan-reviews@chromium.org, tkent@chromium.org, vegorov@chromium.org, zerny@chromium.org BUG= Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176307

Patch Set 1 #

Patch Set 2 : New QName hash trait #

Total comments: 3

Patch Set 3 : Review feedback #

Total comments: 2

Patch Set 4 : #

Patch Set 5 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+116 lines, -71 lines) Patch
M Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl View 1 2 1 chunk +3 lines, -3 lines 0 comments Download
M Source/core/animation/AnimationTimelineTest.cpp View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/dom/Document.cpp View 1 2 3 4 3 chunks +4 lines, -4 lines 0 comments Download
M Source/core/dom/Element.cpp View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M Source/core/dom/QualifiedName.h View 1 2 3 5 chunks +47 lines, -12 lines 0 comments Download
M Source/core/dom/QualifiedName.cpp View 1 2 3 4 3 chunks +25 lines, -15 lines 0 comments Download
M Source/core/dom/custom/CustomElementRegistry.cpp View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/forms/InputType.cpp View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M Source/core/svg/SVGAngleTearOff.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M Source/core/svg/SVGAnimatedEnumerationBase.cpp View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/svg/SVGLengthListTearOff.h View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/svg/SVGLengthTearOff.h View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/svg/SVGNumberListTearOff.h View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/svg/SVGNumberTearOff.h View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/svg/SVGPathSegListTearOff.h View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/svg/SVGPointListTearOff.h View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/svg/SVGPointTearOff.h View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/svg/SVGPreserveAspectRatioTearOff.h View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/svg/SVGRectTearOff.h View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/svg/SVGStringListTearOff.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/svg/SVGTransformListTearOff.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/svg/SVGTransformListTearOff.cpp View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/svg/SVGTransformTearOff.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/svg/properties/SVGAnimatedProperty.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/svg/properties/SVGAnimatedProperty.cpp View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/svg/properties/SVGListPropertyTearOffHelper.h View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/svg/properties/SVGPropertyTearOff.h View 1 2 3 4 3 chunks +3 lines, -3 lines 0 comments Download
M Source/core/svg/properties/SVGPropertyTearOff.cpp View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/web/WebPageSerializer.cpp View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 29 (0 generated)
wibling-chromium
6 years, 6 months ago (2014-06-03 13:45:30 UTC) #1
haraken
Another idea might be to simplify the data structure. Currently we're using: HeapHashMap<pair<WeakMember<SVGElement>, QualifiedName>, Member<HeapLinkedHashSet<WeakMember<SVGSMILElement>>>> ...
6 years, 6 months ago (2014-06-03 14:55:33 UTC) #2
Mads Ager (chromium)
On 2014/06/03 14:55:33, haraken wrote: > Another idea might be to simplify the data structure. ...
6 years, 6 months ago (2014-06-04 06:37:52 UTC) #3
Mads Ager (chromium)
On 2014/06/04 06:37:52, Mads Ager (chromium) wrote: > On 2014/06/03 14:55:33, haraken wrote: > > ...
6 years, 6 months ago (2014-06-04 07:01:41 UTC) #4
haraken
On 2014/06/04 07:01:41, Mads Ager (chromium) wrote: > On 2014/06/04 06:37:52, Mads Ager (chromium) wrote: ...
6 years, 6 months ago (2014-06-04 07:10:35 UTC) #5
Mads Ager (chromium)
On 2014/06/04 07:10:35, haraken wrote: > On 2014/06/04 07:01:41, Mads Ager (chromium) wrote: > > ...
6 years, 6 months ago (2014-06-04 07:13:43 UTC) #6
haraken
On 2014/06/04 07:13:43, Mads Ager (chromium) wrote: > On 2014/06/04 07:10:35, haraken wrote: > > ...
6 years, 6 months ago (2014-06-04 07:20:59 UTC) #7
haraken
As far as I understand, this hash map is a rare, crazy case, so probably ...
6 years, 6 months ago (2014-06-04 07:21:49 UTC) #8
Mads Ager (chromium)
On 2014/06/04 07:21:49, haraken wrote: > As far as I understand, this hash map is ...
6 years, 6 months ago (2014-06-04 07:32:53 UTC) #9
blink-reviews
FYI. I am looking into changing the QualifiedName's hash trait to work without the nullQName ...
6 years, 6 months ago (2014-06-04 07:41:32 UTC) #10
wibling-chromium
"I" in the above reply happens to be me:)
6 years, 6 months ago (2014-06-04 07:49:33 UTC) #11
haraken
> > Yes, it is probably OK in this case, I agree. However, we need ...
6 years, 6 months ago (2014-06-04 07:52:59 UTC) #12
blink-reviews
The difference between AtomicString/String and QualifiedName is that AtomicString/String can have a m_impl which is ...
6 years, 6 months ago (2014-06-04 08:04:41 UTC) #13
wibling-chromium
PTAL.
6 years, 6 months ago (2014-06-04 10:36:40 UTC) #14
zerny-chromium
It is really a shame that this "empty" value is forced on collection elements. lgtm.
6 years, 6 months ago (2014-06-04 10:51:06 UTC) #15
Mads Ager (chromium)
LGTM, but we let's wait and see if eseidel@ has comments based on experience with ...
6 years, 6 months ago (2014-06-04 10:58:10 UTC) #16
haraken
LGTM, but experts should confirm. Given comment #13, I like the idea of addressing this ...
6 years, 6 months ago (2014-06-05 00:53:10 UTC) #17
esprehn
lgtm, but please fix the style before landing. I'm also somewhat concerned that adding all ...
6 years, 6 months ago (2014-06-05 00:58:43 UTC) #18
esprehn
Actually, what if we the same thing for the nullQName that we do for AtomicStrings ...
6 years, 6 months ago (2014-06-05 01:01:08 UTC) #19
wibling-chromium
On 2014/06/05 01:01:08, esprehn wrote: > Actually, what if we the same thing for the ...
6 years, 6 months ago (2014-06-10 10:59:26 UTC) #20
wibling-chromium
PTAL
6 years, 6 months ago (2014-06-12 10:22:42 UTC) #21
haraken
LGTM. Elliott might want to take another look. https://codereview.chromium.org/311803003/diff/40001/Source/core/dom/QualifiedName.h File Source/core/dom/QualifiedName.h (right): https://codereview.chromium.org/311803003/diff/40001/Source/core/dom/QualifiedName.h#newcode37 Source/core/dom/QualifiedName.h:37: // ...
6 years, 6 months ago (2014-06-12 10:57:15 UTC) #22
wibling-chromium
Thanks for the review! I will wait for Elliott to take a look as well. ...
6 years, 6 months ago (2014-06-12 11:02:05 UTC) #23
wibling-chromium
Elliott, do you have time to take a second look at this?
6 years, 6 months ago (2014-06-17 10:51:41 UTC) #24
esprehn
lgtm, awesome!
6 years, 6 months ago (2014-06-17 11:09:24 UTC) #25
wibling-chromium
On 2014/06/17 11:09:24, esprehn wrote: > lgtm, awesome! Thanks!
6 years, 6 months ago (2014-06-17 11:10:27 UTC) #26
wibling-chromium
The CQ bit was checked by wibling@chromium.org
6 years, 6 months ago (2014-06-17 11:10:32 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wibling@chromium.org/311803003/80001
6 years, 6 months ago (2014-06-17 11:10:50 UTC) #28
commit-bot: I haz the power
6 years, 6 months ago (2014-06-17 11:44:21 UTC) #29
Message was sent while issue was closed.
Change committed as 176307

Powered by Google App Engine
This is Rietveld 408576698