Consider renaming Node.rootNode because of web compatibility issues

RESOLVED FIXED in Firefox 53

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: smaug, Assigned: jdai)

Tracking

({dev-doc-complete, site-compat})

36 Branch
mozilla53
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox48- unaffected, firefox49+ unaffected, firefox53 fixed)

Details

(Whiteboard: btpp-fixlater [webcompat])

Attachments

(2 attachments, 2 obsolete attachments)

See https://github.com/whatwg/dom/issues/241


...or we may need to first disable the feature, before it is decided what the new name will be.

Disabling would mean just adding [Pref=some.preference] to the .webidl and having that preference being false by default. And then changing rootNode.html.ini
so that we set the pref when testing.
We may want to keep the pref on in non-release builds, so aurora and nightly
Summary: Consider rename Node.rootNode because of web compatibility issues → Consider renaming Node.rootNode because of web compatibility issues
smaug: Do you know if this affects 47? Can you also clarify what the pref does and will you work on it? Tracking for 48 and 49.
Flags: needinfo?(bugs)
I was hoping jocelyn could take a look at this. The property was added in Bug 1254956.
And since that landed for FF48, this shouldn't affect to 47.

The spec work on this is still ongoing. See https://github.com/whatwg/dom/issues/241
So it is not clear what the property will be called. 
(rniwa is from Apple and hayatoito from Google)
Flags: needinfo?(bugs) → needinfo?(joliu)
Whiteboard: btpp-fixlater
Thanks for the notice, I will take care of this.
Looks like they're still discussing possible names at this moment.
Safari already unshipped this feature but Blink doesn't have a plan to unship it yet.
I would like to turn it off only for the beta/release build first (though it won't be any difference before we ship FF48 to beta channel) unless we see more bug reports on this or if there's a consensus that all browsers should unship this before the name is finalized from the web compatibility point of view.
Assignee: nobody → joliu
Flags: needinfo?(joliu)
yeah, that sounds good, disabled on release/beta, enabled on aurora/nightly.
And once we know what the new name will be, change the name and then possibly enable on beta/release too.
See Also: → 1270387
I will leave this bug for adopting the change in the spec, open Bug 1270387 for disabling this feature in release builds.
The conclusion of https://github.com/whatwg/dom/issues/241 is removing this API, and the changeset has already been reverted in https://github.com/whatwg/dom/commit/42a45eb0d1af13e510620b6b29696997f0c7aa3d.

I guess our action here will be removing this entirely including the preference added in Bug 1270387 from FF48, FF49, and FF50.
And the spec was changed again...
(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #8)
> And the spec was changed again...

Yes... Would need to revise to getRootNode() then.
ref: https://github.com/whatwg/dom/pull/248
Hi Olli,
Since Bug 1270387 has disabled this feature in release builds, and also to consider the release date of FF48, I wonder if we still think this bug is necessary for FF48, and think we may want to consider setting the tracking flag "status-firefox48" from "affected" to either "fix-optional" or "wontfix," according to the explanation in https://wiki.mozilla.org/Platform#Process
Flags: needinfo?(bugs)
This isn't needed for FF48, since .rootNode is disabled.
Fixing this should mean also removing the pref for .rootNode.
Hopefully the new method name is web compatible enough that we won't need to disable it.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] (on vacation for couple of days) from comment #11)
> This isn't needed for FF48, since .rootNode is disabled.
> Fixing this should mean also removing the pref for .rootNode.
> Hopefully the new method name is web compatible enough that we won't need to
> disable it.

Thanks Olli. I will change "status-firefox48" to wontfix so that this is removed from Release manager's radar.
Hi Hsin-Yi,
Is there any updates on this?
Flags: needinfo?(htsai)
(In reply to Gerry Chang [:gchang] from comment #13)
> Hi Hsin-Yi,
> Is there any updates on this?

No... 
Since the original feature is disabled in release builds, firefox49 is also unaffected. Is this what you want to understand?
Flags: needinfo?(htsai)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #14)
> (In reply to Gerry Chang [:gchang] from comment #13)
> > Hi Hsin-Yi,
> > Is there any updates on this?
> 
> No... 
> Since the original feature is disabled in release builds, firefox49 is also
> unaffected. Is this what you want to understand?

Yes. Thanks for clarification.
Depends on: 1303802
Whiteboard: btpp-fixlater → btpp-fixlater [webcompat]
Hi John, could you please take care of this? As this causes web-compat issue, I think we'd consider to handle it soon-ish. :)
Flags: needinfo?(jdai)
Priority: -- → P2
Assignee

Updated

2 years ago
Assignee: yrliou → jdai
Flags: needinfo?(jdai)
Assignee

Comment 19

2 years ago
- Revise rootNode to getRootNode(). 
- Remove Pref=dom.node.rootNode.enabled.
- Update getRootNode() Web Platform Tests expected results.
- Address comment #16.

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb31190d5e61a97131ec46485fc3c86897690df1&filter-tier=1&group_state=expanded
Attachment #8822586 - Flags: review?(bugs)
Comment on attachment 8822586 [details] [diff] [review]
Bug 1269155 - Revise Node.rootNode to Node.getRootNode.

>+nsINode* nsINode::GetRootNode(const mozilla::dom::GetRootNodeOptions& aOptions)
Nit, return value type to its own line, similar to the method 'SubtreeRoot' below.
And I think you don't need mozilla::dom:: here.
There is after all 
using namespace mozilla;
using namespace mozilla::dom;
in the file

>+{
>+  if (aOptions.mComposed) {
>+    return AsContent()->GetContainingShadow();
>+  }
This is not right. First, the method can be called on non-nsIContent objects, so AsContent() would crash. (just call the method on an document object) and GetContainingShadow()
returns ShadowRoot, but we want the shadow including root node of the tree. So you want shadow host's root node recursively. And in practice we want a fast path for the case when
node is in document, effectively something like if (aOptions.mComposed && IsInComposedDoc()) { return OwnerDoc() ); }
The method should be O(1) in other cases expect when we're dealing with shadow nodes which aren't in document

>-  nsINode* RootNode() const
>-  {
>-    return SubtreeRoot();
>-  }
>+  /*
>+   * Get context object’s shadow-including root if options’s composed is true,
>+   * and context object’s root otherwise.
Hmm, there are some weird characters here after object. Want to fix?
Attachment #8822586 - Flags: review?(bugs) → review-
Assignee

Comment 21

2 years ago
(In reply to Olli Pettay [:smaug] from comment #20)
> Comment on attachment 8822586 [details] [diff] [review]
> Bug 1269155 - Revise Node.rootNode to Node.getRootNode.
> 
> >+nsINode* nsINode::GetRootNode(const mozilla::dom::GetRootNodeOptions& aOptions)
> Nit, return value type to its own line, similar to the method 'SubtreeRoot'
> below.
> And I think you don't need mozilla::dom:: here.
> There is after all 
> using namespace mozilla;
> using namespace mozilla::dom;
> in the file
> 
> >+{
> >+  if (aOptions.mComposed) {
> >+    return AsContent()->GetContainingShadow();
> >+  }
> This is not right. First, the method can be called on non-nsIContent
> objects, so AsContent() would crash. (just call the method on an document
> object) and GetContainingShadow()
> returns ShadowRoot, but we want the shadow including root node of the tree.
> So you want shadow host's root node recursively. And in practice we want a
> fast path for the case when
> node is in document, effectively something like if (aOptions.mComposed &&
> IsInComposedDoc()) { return OwnerDoc() ); }
> The method should be O(1) in other cases expect when we're dealing with
> shadow nodes which aren't in document

I think I may misunderstand something. Please correct me if I'm wrong. In your review comment,
> The method should be O(1) in other cases expect when we're dealing with shadow nodes which aren't in document
Is this a typo of expect? It should be *except*.
If this isn't a typo, you said we can use 
> if (aOptions.mComposed && IsInComposedDoc()) { return OwnerDoc() ); }
to apply both nodes are in document and aren't in document.
However, IsInComposedDoc() is a method which only make sure nodes are in Document. Could you explain more about why we can use 
> if (aOptions.mComposed && IsInComposedDoc()) { return OwnerDoc() ); }
in document and aren't in document. Thank you.

> 
> >-  nsINode* RootNode() const
> >-  {
> >-    return SubtreeRoot();
> >-  }
> >+  /*
> >+   * Get context object’s shadow-including root if options’s composed is true,
> >+   * and context object’s root otherwise.
> Hmm, there are some weird characters here after object. Want to fix?
Will do.
Yes, except, not expect.


if (aOptions.mComposed && IsInComposedDoc()) { return OwnerDoc() ); }
is only an optimization. We want to have O(1) behavior for the cases when node is in document, even when node is in shadow tree.
We need to still have the recursive root node finding for the case we're in shadow dom and want to find the composed root. That should be hopefully rare case and can't be O(1) in our current setup
Assignee

Comment 23

2 years ago
Hi Olli,
I addressed comment #20 and comment #22. Could you help me to review it again? Thank you.

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c186d777bec090102076475bd3c848005e02d944&group_state=expanded&filter-tier=1
Attachment #8822586 - Attachment is obsolete: true
Attachment #8826516 - Flags: review?(bugs)
Comment on attachment 8826516 [details] [diff] [review]
Bug 1269155 - Revise Node.rootNode to Node.getRootNode. v2


>+
>+    nsINode* node = this;
>+    nsINode* iter = nullptr;
>+    ShadowRoot* shadowRootParent = nullptr;
>+    while(node) {
>+      iter = node;
>+      while ((iter = iter->GetParentNode())) {
>+        node = iter;
>+      }
>+
>+      shadowRootParent = ShadowRoot::FromNode(node);
>+      if (!shadowRootParent) {
>+         break;
>+      }
>+      node = shadowRootParent->GetHost();
>+    }
The inner loop should still use SubtreeRoot(), not GetParentNode() loop.
And in that case it wouldn't be a loop anymore, but just
node = node->SubtreeRoot();
shadowRootParent = ShadowRoot::FromNode(node);
if (!shadowRootParent) {
  break;
}
node = shadowRootParent->GetHost();


With that, r+
Attachment #8826516 - Flags: review?(bugs) → review+
Assignee

Comment 25

2 years ago
Addressed comment #24, and carried r+.
Attachment #8826516 - Attachment is obsolete: true
Attachment #8827095 - Flags: review+

Comment 27

2 years ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14a12ac05a4b
Revise Node.rootNode to Node.getRootNode. r=smaug
Keywords: checkin-needed

Comment 28

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/14a12ac05a4b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

2 years ago
Depends on: 1325357
Blocks: 1325357
No longer depends on: 1325357
Can we uplift this patch to 52, since it appears to fix the HSBC website (bug 1325357)?
Flags: needinfo?(jdai)
Flags: needinfo?(bugs)
How super weird. Do they really depend on this very new DOM feature ?
Not sure why it's broken without this patch, but the pushlog in bug 1325357 comment 40 (which was generated to find what fixed the bug in 53) contains this change.

We could ask the user to run mozregression again to find what broke the site between 51 and 52.
That would be good, to actually understand what broke the site.
(In reply to Olli Pettay [:smaug] from comment #30)
> How super weird. Do they really depend on this very new DOM feature ?

Node.rootNode broke sites in Chrome and WebKit due to scripts relying on some random rootNode property, so possibly just another instance of this new prop clobbering existing code? 

https://bugs.webkit.org/show_bug.cgi?id=157233
https://bugs.chromium.org/p/chromium/issues/detail?id=600950

If anyone has access to HSBC UK, it would be interesting to search for all instances of rootNode on the site, and if possible download the scripts that do have it (and email them to me?).
rootNode was already disabled in bug 1270387, and this bug added getRootNode().
Oh, hmm, Aurora had .rootNode for some time when Nightly didn't, I guess.
Flags: needinfo?(bugs)
> If anyone has access to HSBC UK, it would be interesting to search for all instances of rootNode on the site, and if possible download the scripts that do have it (and email them to me?).

Actually, nevermind.

https://www.security.hsbc.co.uk/gsp/saas/Components/default/resources/script/libraries/dtk/dojo/dojo.js

var root;if(!_484&&_483&&_483.rootNode){_484=_483;root=_484.rootNode;...

That's probably enough to break the site.

So, rootNode was only ever dev edition and nightly, and the HSBC bug only repros in dev edition and nightly...

-#ifdef RELEASE_OR_BETA
-pref("dom.node.rootNode.enabled", false);
-#else
-pref("dom.node.rootNode.enabled", true);
-#endif

And this patch fixes the bug in dev edition and nightly...

Do we need beta (currently 52) uplift if the prefs weren't removed until 53?

(I feel like I might be missing something...)
Assignee

Comment 36

2 years ago
Approval Request Comment
[Feature/Bug causing the regression]: Bug 1325357 causing the regression.
[User impact if declined]: It will enable in firefox 53.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:  none
[Is the change risky?]: minor risk
[Why is the change risky/not risky?]: We need this on firefox 52 to avoid regression.
[String changes made/needed]: none
Flags: needinfo?(jdai)
Attachment #8833933 - Flags: review+
Attachment #8833933 - Flags: approval-mozilla-beta?
I don't understand why we need approval here. HSBC site was broken only in nightly/aurora.
Yes, there's no need to uplift anymore, since the `dom.node.rootNode.enabled` was only enabled in nightly/aurora.
Comment on attachment 8833933 [details] [diff] [review]
(Beta) Bug 1269155 - Revise Node.rootNode to Node.getRootNode. (Final) r=smaug

There seems to be some confusion as to whether this is needed on beta, a- for now...
Attachment #8833933 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.