1269155 - Consider renaming Node.rootNode because of web compatibility issues

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

We may want to keep the pref on in non-release builds, so aurora and nightly

Comment 2

[Marcia Knous [:marcia]]

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.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 4

[Jocelyn Liu [:jocelyn] [:joliu]]

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.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 6

[Jocelyn Liu [:jocelyn] [:joliu]]

I will leave this bug for adopting the change in the spec, open Bug 1270387 for disabling this feature in release builds.

Comment 7

[Jocelyn Liu [:jocelyn] [:joliu]]

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.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

And the spec was changed again...

Comment 9

[Jocelyn Liu [:jocelyn] [:joliu]]

(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

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

Comment 13

[Gerry Chang [:gchang]]

Hi Hsin-Yi,
Is there any updates on this?

Comment 14

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

Comment 15

[Gerry Chang [:gchang]]

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

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Make sure to revert https://hg.mozilla.org/mozilla-central/rev/d2193462357a

Comment 17

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

https://dom.spec.whatwg.org/#dom-node-getrootnode

Comment 19

[John Dai[:jdai]]

Attachment: Bug 1269155 - Revise Node.rootNode to Node.getRootNode.

- 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

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 21

[John Dai[:jdai]]

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

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 23

[John Dai[:jdai]]

Attachment: Bug 1269155 - Revise Node.rootNode to Node.getRootNode. v2

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

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

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+

Comment 25

[John Dai[:jdai]]

Attachment: Bug 1269155 - Revise Node.rootNode to Node.getRootNode. (Final) r=smaug

Addressed comment #24, and carried r+.

Comment 26

[John Dai[:jdai]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38b33f0b6a5e5697c016ed0268745c2dea529c77&filter-tier=1&group_state=expanded

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14a12ac05a4b
Revise Node.rootNode to Node.getRootNode. r=smaug

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/14a12ac05a4b

Comment 29

[Marco Castelluccio [:marco]]

Can we uplift this patch to 52, since it appears to fix the HSBC website (bug 1325357)?

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

How super weird. Do they really depend on this very new DOM feature ?

Comment 31

[Marco Castelluccio [:marco]]

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.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

That would be good, to actually understand what broke the site.

Comment 33

[Mike Taylor [:miketaylr]]

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

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 35

[Mike Taylor [:miketaylr]]

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

Comment 36

[John Dai[:jdai]]

Attachment: (Beta) Bug 1269155 - Revise Node.rootNode to Node.getRootNode. (Final) r=smaug

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

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

I don't understand why we need approval here. HSBC site was broken only in nightly/aurora.

Comment 38

[Marco Castelluccio [:marco]]

Yes, there's no need to uplift anymore, since the `dom.node.rootNode.enabled` was only enabled in nightly/aurora.

Comment 39

[Julien Cristau [:jcristau]]

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

Comment 40

[Chris Mills [:cmills]]

I've updated browser support info in appropriate places and made sure the method is property documented:

https://developer.mozilla.org/en-US/docs/Web/API/Node
https://developer.mozilla.org/en-US/docs/Web/API/Node/getRootNode
https://developer.mozilla.org/en-US/docs/Web/API/Node/rootNode

I've also added a note to the Fx53 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/53