Bug 77700

Created attachment 125270 [details] Mostly a patch Let's see what EWS thinks of this.

Comment on attachment 125270 [details] Mostly a patch Attachment 125270 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11420198 New failing tests: http/tests/security/cross-origin-css-primitive.html fast/css/css-value-wrapper-sameness.html

Comment on attachment 125270 [details] Mostly a patch Makes sense to me. While this is an observable behavior change, the case is so obscure it seems unlikely to matter in practice. The current behavior with wrapper identities is already rather surprising and that hasn't caused problems.

It might make sense to do the wrapper behavior change separately from the value pool refactoring.

Do we have a way to check for performance impact? In scripts the repeatedly access the same value, this would cause additional garbage to collect.

(In reply to comment #5) > Do we have a way to check for performance impact? In scripts the repeatedly access the same value, this would cause additional garbage to collect. You are right of course, but I don't know of anyone or anything using the CSSValue interface to a greater extent in production. There is a single call to getPropertyCSSValue() in our Web Inspector JS code, so that's not helpful either.

I think the only way to find out is to try it. Computed styles (which are probably most of the real world use due to wider engine support) don't consume significant amounts of memory so if needed we could use unique (non-pool) CSSValues there and allow wrapper caching for those.

(In reply to comment #7) > I think the only way to find out is to try it. > > Computed styles (which are probably most of the real world use due to wider engine support) don't consume significant amounts of memory so if needed we could use unique (non-pool) CSSValues there and allow wrapper caching for those. Indeed. Like we discussed on IRC, I'm tempted to move towards supporting the CSSValue interface only for computed styles. This would match Gecko, and allow us to simplify non-computed styles further. Completely outside the scope of this bug obviously.

Created attachment 127738 [details] Wrapper part of the change, amended for V8.

(In reply to comment #9) > Created an attachment (id=127738) [details] > Wrapper part of the change, amended for V8. This piece landed in <http://trac.webkit.org/changeset/108195>.

I believe this change is incompatible with WebIDL. Do you think there's a specification that comments on what's correct behavior here? > Because the CSSValue DOM interface is very rarely used and is poorly supported by other engines I see some unclear thinking here. These arguments cut equally against making performance improvements and preserving backwards-compatibility. If nobody uses or implements this API, neither behavior nor performance matter. > This will allow us to make CSSValuePool global It seems like you're trading more wrapper allocation for less underlying value allocation. Why is this trade a net win?

(In reply to comment #11) > I believe this change is incompatible with WebIDL. Do you think there's a specification that comments on what's correct behavior here? Not that I know of. If WebIDL does specify this behavior, I suspect we have a number of methods violating it by always returning a unique underlying value resulting in a unique wrapper. > > Because the CSSValue DOM interface is very rarely used and is poorly supported by other engines > > I see some unclear thinking here. These arguments cut equally against making performance improvements and preserving backwards-compatibility. If nobody uses or implements this API, neither behavior nor performance matter. The "poorly supported" part refers to the fact that we are the only engine that implements getPropertyCSSValue() for non-computed style declarations. Furthermore, in the new CSSOM draft currently in development, CSSValue is basically stripped down to { .cssText } Most importantly, this is by no means the intended end state. The per-document value caching was blocking Antti's ongoing CSS refactoring so he asked me to finish and land it. Over the next couple of weeks, we'll see the introduction of a new internal CSS value class (tentatively named StyleValue.) CSSValue wrapper creation will be managed by their logical owners, i.e the CSSStyleDeclaration they are extracted from. This will change our behavior slightly once again. > > This will allow us to make CSSValuePool global > > It seems like you're trading more wrapper allocation for less underlying value allocation. Why is this trade a net win? Because the vast majority of pages never instantiate a single CSSValue wrapper, but hundreds if not thousands of CSSValue objects.

Reverted r108195 for reason: Lots of failing ASSERT()s on v8 bots, requested by kling on #webkit Committed r108200: <http://trac.webkit.org/changeset/108200>

I couldn't get a full list of layout tests that failed ASSERT()s in v8 debug since the bots early-exited after 20 crashes, but here's an example ASSERT() failure from a chromium linux debug bot on test fast/css/CSSPrimitiveValue-exceptions.html: ASSERTION FAILED: !m_map.contains(obj) third_party/WebKit/Source/WebCore/bindings/v8/V8DOMMap.h(91) : void WebCore::WeakReferenceMap<KeyType, ValueType>::set(KeyType*, v8::Persistent<ValueType>) [with KeyType = void, ValueType = v8::Object] 1 0x1500864 2 0x1a315b5 3 0x19067c2 4 0x1906800 5 0x253747e 6 0x1878c0f 7 0x198d3bc 8 0x8c476f 9 0x8bf206 10 0x8bf1d7 11 0x1ca037e0424e [41817:41817:15198725650916:ERROR:process_util_posix.cc(142)] Received signal 11 base::debug::StackTrace::StackTrace() [0x73193e] base::(anonymous namespace)::StackDumpSignalHandler() [0x6eb4d9] 0x7f8f06ba5af0 WebCore::WeakReferenceMap<>::set() [0x150086e] WebCore::V8CSSPrimitiveValue::wrapSlow() [0x1a315b5] WebCore::V8CSSPrimitiveValue::wrap() [0x19067c2] WebCore::toV8() [0x1906800] WebCore::toV8() [0x253747e] WebCore::toV8() [0x1878c0f] WebCore::CSSStyleDeclarationInternal::getPropertyCSSValueCallback() [0x198d3bc] v8::internal::HandleApiCallHelper<>() [0x8c476f] v8::internal::Builtin_Impl_HandleApiCall() [0x8bf206] v8::internal::Builtin_HandleApiCall() [0x8bf1d7] 0x1ca037e0424e None

Comment on attachment 125270 [details] Mostly a patch Cleared Antti Koivisto's review+ from obsolete attachment 125270 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Created attachment 136350 [details] Globalization patch

Comment on attachment 136350 [details] Globalization patch r=me

Committed r113647: <http://trac.webkit.org/changeset/113647>