Bisected to:

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

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

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...!!

I can confirm this seems to be caused by the ContextIsValid() check fs@ mentioned.

+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?

> 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.)

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?

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.

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? 

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.

Fix up for review: https://chromium-review.googlesource.com/c/chromium/src/+/1100836

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

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?

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

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.

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

has this been verified in canary yet?

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.

> 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.

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

Also verified on Windows (69.0.3464.0).

Approving merge to M68 branch 3440 based on comments #16, #21 & #23. Please merge ASAP. Thank you.

+abdulsyed@ as FYI

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

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...!!

Deleted: 852190.mp4 458 KB

	852190.mp4 458 KB View Download