New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment
link

Issue 852190: Style not applied on SVG Path extracted from Object tag

Reported by duluc.ma...@gmail.com, Jun 13 2018

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:60.0) Gecko/20100101 Firefox/60.0

Steps to reproduce the problem:
1. Create an Object tag, to load an SVG
2. When the SVG is loaded, access it via contentDocument property on the Object tag and extract it from it to place it next to it.
3. Pick any Path tag inside it to update any style property

Example set up : http://maxwellito.github.io/drafts/vivus-198/bug.html

What is the expected behavior?
The new style is set and applied.

What went wrong?
The values should be applied to the element.

Did this work before? Yes 66

Does this work in other browsers? Yes

Chrome version: 67.0.3396.79 (Official Build) (64-bit)  Channel: stable
OS Version: OS X 10.12
Flash Version: 30.0.0.113
 

Comment 1 by viswa.karala@chromium.org, Jun 13 2018

Labels: Needs-Bisect Needs-Triage-M67

Comment 2 by tkent@chromium.org, Jun 13 2018

Components: Blink>SVG

Comment 3 by f...@opera.com, Jun 13 2018

Components: -Blink>SVG Blink>CSS
Labels: -Needs-Bisect
Owner: raphael....@intel.com
Status: Assigned (was: Unconfirmed)
Bisected to:

https://chromium.googlesource.com/chromium/src/+log/718e2ab27bdc9c8907890ef01885ee58199a72f6..9a54951119a1f2493b994e24b0524512c97584e3

5b84e88a070e40a1a74576a6cf41109d29a8035a seems relevant (probably the added ContextIsValid() check.)

Comment 4 by krajshree@chromium.org, Jun 13 2018

Labels: -Pri-2 hasbisect-per-revision ReleaseBlock-Stable Triaged-ET RegressedIn-67 M-67 Target-67 FoundIn-67 Target-68 Target-69 FoundIn-69 FoundIn-68 OS-Linux OS-Windows Pri-1
Able to reproduce the issue on Windows 10, mac 10.13.3 and Ubuntu 17.10 using chrome reported version #67.0.3396.79 and latest canary #69.0.3457.0.

Bisect Information:
=====================
Good build: 67.0.3387.0
Bad Build : 67.0.3388.0

Change Log URL: 
https://chromium.googlesource.com/chromium/src/+log/02ae8f5f6cb3389b7ffb841592f91035f13906a5..5b84e88a070e40a1a74576a6cf41109d29a8035a

From the above change log suspecting below change
Change-Id: I76171bb4c2ae23fc672748dc11b26094f16e573e
Reviewed-on: https://chromium-review.googlesource.com/984232

raphael.kubo.da.costa@ - Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner.
Note: Adding stable blocker for M-67 as it seems to be a recent regression. Please feel free to remove the same if not appropriate.

Thanks...!!

Comment 5 by raphael....@intel.com, Jun 13 2018

Components: Blink>Bindings
Status: Started (was: Assigned)
I can confirm this seems to be caused by the ContextIsValid() check fs@ mentioned.

Comment 6 by raphael....@intel.com, Jun 13 2018

Cc: yukishiino@chromium.org
+yukishiino for additional input

https://chromium-review.googlesource.com/984232 added a call to ScriptState::ContextIsValid() check to CSSStyleDeclaration's named setter that is currently failing in the test case because |per_context_data_| is null. Previously, the manual bindings didn't call ContextIsValid() and just derived an ExecutionContext from info.Holder()->CreationContext() in order to call ExecutionContext::GetSecureContextMode().

What's happening here is that the SVG is first loaded into an <object> that's later destroyed through the call to Node.removeChild(). This call causes ScriptState::DisposePerContextData() to be called, even though the v8::Context associated with the given ScriptState is still valid.

While just removing the call to ContextIsValid() is easy enough, I'm still wondering if the rest is working as planned: when a Node is moved to adopted by a new Document, should it get a new |per_context_data_|, or should the existing one at least not be disposed of when the former document gets removed?

Comment 7 by yukishiino@chromium.org, Jun 13 2018

> While just removing the call to ContextIsValid() is easy enough, I'm still wondering if the rest is working as planned: when a Node is moved to adopted by a new Document, should it get a new |per_context_data_|, or should the existing one at least not be disposed of when the former document gets removed?

This is a really complicated and tough problem.

Discarding |per_context_data_| here and there is a compromise, otherwise the current Blink GC and V8 GC cannot collaborate well enough and there is no way to discard v8::Context due to strong cyclic references between Blink and V8.  We knew that no web standard says that we should discard |per_context_data_|, however, we cannot hold it unconditionally.

In short, discarding |per_context_data_| is wrong, but we don't have any other better way so far.

The coming "Unified GC" should improve the situation much better, and then we will no longer need to discard |per_context_data_|.  (Having said that, some of people like an idea to discard things eagerly in order to keep the memory usage low.)


By the way, my understanding about Firefox is very shallow, but I heard that, when you moved a Node into another Document, Firefox changes the Node's associated realm, etc. to the new Document.  In our words, the moved Node no longer has a reference to the old Document / old ScriptState / old ExecutionContext / old v8::Context.  I think that this way could be an option, but ECMAScript doesn't suppose that an ES Function.[Realm] changes runtime.  So, this is a bit tricky.


Summary is:
a) We knew we're wrong, but we cannot fix it immediately.
b) Once the Unified GC is supported, probably we can fix the "discard context" problem.  (I hope so.)

Comment 8 by raphael....@intel.com, Jun 13 2018

Thanks for the explanation.

While the issue is properly worked out, instead of just removing the check I was considering replacing it with a CHECK/DCHECK that only asserts that the ScriptState's v8::Context is valid (not sure if ScriptState::GetContext() would do that or if I need to add a new method). Does that sound sane to you?

Comment 9 by yukishiino@chromium.org, Jun 13 2018

Given that AnonymousNamedSetter is called directly from the bindings, |script_state| should be somewhat usable state at least.  If the problem is common, the generated bindings should have some general check and handle error cases.  So, let's assume |script_state| is somewhat okay even if it's not fully equipped.

There are two use cases of |script_state|:

a) script_state->GetIsolate()
This must be okay.  v8::Isolate is invalidated at very end of life.

b) ExecutionContext::From(script_state)
This could be nullptr if |script_state| is not that okay.

What about checking ExecutionContext::From(script_state)?

I'm open to other options.  However, I'd think that, in general, Web APIs operate on DOM objects and its document tree.  So, we should care about the valid states of the document tree and DOM objects rather than v8::Context.  AnonymousNamedSetter is called after the binding layer took care of the bindings, so AnonymousNamedSetter should ideally be able to concentrate on their own task (in terms of DOM object).  If AnonymousNamedSetter needed to care too much about ScriptState, maybe the design would be bad.

I don't object to have checks for safety, but I'd check ExecutionContext::From(script_state) rather than v8::Context.

Comment 10 by gov...@chromium.org, Jun 13 2018

M67 has been out since 05/29, currently at 50% for Windows and Mac, 100% Linux. Plan is to ramp up to 100% tomorrow if all goes well. And at the moment, there is no plan to do stable respin unless extremely critical issues arise.

yukishiino@, is this indeed M67 stable blocker for further roll out?

Comment 11 by yukishiino@chromium.org, Jun 13 2018

Labels: -ReleaseBlock-Stable
This is indeed a regression in M67, but I don't think that this is that critical issue to block M67 stable release.  Thus, I remove ReleaseBlock-Stable label.

Comment 13 by bugdroid1@chromium.org, Jun 14 2018

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b179b0b880afa692b4ed938ca7c232259dbccb0c

commit b179b0b880afa692b4ed938ca7c232259dbccb0c
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Thu Jun 14 16:58:11 2018

CSSStyleDeclaration: Stop calling ScriptState::ContextIsValid() in the setter

When the custom bindings were removed in commit 5b84e88a0 ("Remove custom
bindings for CSSStyleDeclaration"), the new anonymous setter code added a
check for ScriptState::ContextIsValid() to ensure it had received a valid
|script_state|.

It turns out this check is too strict, as in addition to verifying a
ScriptState has an associated v8::Context it also asserts the ScriptState
has a valid |per_context_data_|.

This is valid in most cases, but when a node is moved across different
documents and its previous document gets removed its |per_context_data_| is
disposed of and ScriptState::ContextIsValid() fails.

Since the anonymous setter is only invoked by the bindings layer, we can
assume it is passed a ScriptState that is in a minimally usable state, so it
is possible to relax the ContextIsValid() check and only make sure we can
get a valid ExecutionContext from the ScriptState.

Bug:  852190 
Change-Id: I307de0e003e5258bf1f670b26f1dc86e4dae9286
Reviewed-on: https://chromium-review.googlesource.com/1100836
Commit-Queue: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567310}
[add] https://crrev.com/b179b0b880afa692b4ed938ca7c232259dbccb0c/third_party/WebKit/LayoutTests/external/wpt/css/cssom/style-attr-update-across-documents.html
[modify] https://crrev.com/b179b0b880afa692b4ed938ca7c232259dbccb0c/third_party/blink/renderer/core/css/css_style_declaration.cc

Comment 14 by raphael....@intel.com, Jun 14 2018

This should go into M68 after we verify it doesn't break the canary channel. Should I take care of the merge or is it better for a Googler to do so?

Comment 15 by yukishiino@chromium.org, Jun 15 2018

The merge shouldn't be restricted to a specific company, IMHO.  Please go ahead.  However, if you had any trouble, I'm happy to help anything.  :)

The M68 branch is refs/branch-heads/3440 due to http://dev.chromium.org/developers/how-tos/drover

Comment 16 by raphael....@intel.com, Jun 15 2018

Labels: Merge-Request-68
Requesting permission to merge https://chromium-review.googlesource.com/1100836 into branch-heads/3440. It fixes a regression introduced in M67 that prevented the style of elements from being updated correctly under certain conditions (nodes moving across documents).

The CL is part of 69.0.3460.0, so I'm not sure if we need to wait for another canary before making a decision.

Comment 17 by sheriffbot@chromium.org, Jun 15 2018

Project Member
Labels: -Merge-Request-68 Hotlist-Merge-Review Merge-Review-68
This bug requires manual review: M68 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 18 by abdulsyed@google.com, Jun 16 2018

has this been verified in canary yet?

Comment 19 by raphael....@intel.com, Jun 16 2018

What's the process for that? The CL was landed about 2 days ago, and it looks like it's been in the past 2-3 canaries.

Comment 20 by yukishiino@chromium.org, Jun 18 2018

> What's the process for that?

This issue is easy to repro.  Please confirm that this issue is fixed on the canary that includes your patch, and report the result here (= update this issue with the result).  That's all.

Comment 21 by raphael....@intel.com, Jun 18 2018

Thanks. I've verified everything works again on the Android canary (69.0.3462.0).

Comment 22 by abdulsyed@google.com, Jun 18 2018

Labels: OS-Android

Comment 23 by raphael....@intel.com, Jun 18 2018

Also verified on Windows (69.0.3464.0).

Comment 24 by gov...@chromium.org, Jun 18 2018

Cc: abdulsyed@chromium.org
Labels: -Merge-Review-68 Merge-Approved-68
Approving merge to M68 branch 3440 based on comments #16, #21 & #23. Please merge ASAP. Thank you.

+abdulsyed@ as FYI

Comment 25 by bugdroid1@chromium.org, Jun 18 2018

Project Member
Labels: -merge-approved-68 merge-merged-3440
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/73dbd68c995bf6c7e7584768aa1ccb7d1cab68e5

commit 73dbd68c995bf6c7e7584768aa1ccb7d1cab68e5
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Mon Jun 18 21:25:58 2018

CSSStyleDeclaration: Stop calling ScriptState::ContextIsValid() in the setter

When the custom bindings were removed in commit 5b84e88a0 ("Remove custom
bindings for CSSStyleDeclaration"), the new anonymous setter code added a
check for ScriptState::ContextIsValid() to ensure it had received a valid
|script_state|.

It turns out this check is too strict, as in addition to verifying a
ScriptState has an associated v8::Context it also asserts the ScriptState
has a valid |per_context_data_|.

This is valid in most cases, but when a node is moved across different
documents and its previous document gets removed its |per_context_data_| is
disposed of and ScriptState::ContextIsValid() fails.

Since the anonymous setter is only invoked by the bindings layer, we can
assume it is passed a ScriptState that is in a minimally usable state, so it
is possible to relax the ContextIsValid() check and only make sure we can
get a valid ExecutionContext from the ScriptState.

Bug:  852190 
Change-Id: I307de0e003e5258bf1f670b26f1dc86e4dae9286
Reviewed-on: https://chromium-review.googlesource.com/1100836
Commit-Queue: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#567310}(cherry picked from commit b179b0b880afa692b4ed938ca7c232259dbccb0c)
Reviewed-on: https://chromium-review.googlesource.com/1105257
Reviewed-by: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Cr-Commit-Position: refs/branch-heads/3440@{#419}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[add] https://crrev.com/73dbd68c995bf6c7e7584768aa1ccb7d1cab68e5/third_party/WebKit/LayoutTests/external/wpt/css/cssom/style-attr-update-across-documents.html
[modify] https://crrev.com/73dbd68c995bf6c7e7584768aa1ccb7d1cab68e5/third_party/blink/renderer/core/css/css_style_declaration.cc

Comment 26 by raphael....@intel.com, Jun 18 2018

Labels: -Target-67
Status: Fixed (was: Started)

Comment 27 by krajshree@chromium.org, Jul 3 2018

Labels: TE-Verified-69.0.3480.0 TE-Verified-M69 TE-Verified-M68 TE-Verified-68.0.3440.42
Able to reproduce the issue on Mac 10.13.3 using chrome reported version #67.0.3396.79

Verified the fix on Mac 10.13.3, Win-10 and Ubuntu 17.10 using Chrome beta version #68.0.3440.42 and latest canary #69.0.3480.0 as per the comment #0.
Attaching screen cast for reference.
Observed that new style is set and applied as expected.
Hence, the fix is working as expected. 
Adding the verified labels.

Thanks...!!
852190.mp4
458 KB View Download

Sign in to add a comment