Closed Bug 1326028 Opened 7 years ago Closed 6 years ago

customElements.define must upgrade custom element in shadow-including tree order

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: edgar, Assigned: mrbkap)

References

Details

(Whiteboard: dom-ce-m4 [webcompat])

Attachments

(3 files)

bug 1326028 - Upgrade elements in the right order.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1326028 - Allow moving a reference into nsInterfaceHashtable.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1326028 - We now upgrade elements in the proper order.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Priority: -- → P3
Whiteboard: dom-ce-m4
Blocks: 1419323
Summary: Upgrade custom element in shadow-including tree order → customElements.define must upgrade custom element in shadow-including tree order
This is pretty unfortunate. We have a list of elements (in random order) that we need to visit in order and no way of comparing their positions in the DOM in O(1) time.
Assignee: nobody → mrbkap
Comment on attachment 8952905 [details]
bug 1326028 - Upgrade elements in the right order.

I need to test this more (especially with shadow trees involved) but it seems to work. Once I've tested this and added a couple of WPTs, I'll ask for review.

Olli, does this look reasonable, to you?
Attachment #8952905 - Flags: feedback?(bugs)
Comment on attachment 8952905 [details]
bug 1326028 - Upgrade elements in the right order.

https://reviewboard.mozilla.org/r/222152/#review228078


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/base/CustomElementRegistry.cpp:474
(Diff revision 1)
> +                                 nsIDocument* aDoc)
> +  : mDoc(aDoc)
> +  , mCandidates(aCandidates.Length())
> +{
> +  MOZ_ASSERT(mDoc);
> +  for (auto candidate : aCandidates) {

Warning: Loop variable is copied but only used as const reference; consider making it a const reference [clang-tidy: performance-for-range-copy]

  for (auto candidate : aCandidates) {
            ^
       const  &
Comment on attachment 8952905 [details]
bug 1326028 - Upgrade elements in the right order.

https://reviewboard.mozilla.org/r/222152/#review228344

this all is of course a bit unfortunate from performance point of view.

Could we at least optimize out the case when there is just one candidate?

::: dom/base/CustomElementRegistry.cpp:480
(Diff revision 1)
> +    nsCOMPtr<Element> elem = do_QueryReferent(candidate);
> +    if (!elem) {
> +      continue;
> +    }
> +
> +    mCandidates.Put(elem.get(), elem);

So this ends up addreffing elem again.
Could we possibly use a hashtable which doesn't use internally nsCOMPtr/RefPtr. I don't see reason to use

::: dom/base/CustomElementRegistry.cpp:494
(Diff revision 1)
> +    if (!Traverse(child->AsElement(), orderedElements)) {
> +      break;
> +    }
> +  }
> +
> +  return orderedElements;

don't you end up copying the array here.
(assuming no compiler optimizations are happening)

::: dom/base/CustomElementRegistry.cpp:509
(Diff revision 1)
> +
> +  if (mCandidates.Count() == 0) {
> +    return false;
> +  }
> +
> +  for (Element* child = aRoot->GetFirstElementChild(); child; child = child->GetNextElementSibling()) {

So this stuff doesn't enter to shadow DOM at all.

https://dom.spec.whatwg.org/#shadow-including-preorder-depth-first-traversal
Attachment #8952905 - Flags: feedback?(bugs) → feedback+
(In reply to Olli Pettay [:smaug] from comment #5)
> Could we at least optimize out the case when there is just one candidate?

Yeah. I'd also like to add telemetry to understand how many elements we end up upgrading on average. If we're consistently seeing groups of less than 4, it seems like it would be faster to manually compare the elements without touching every single other node in the document, but I don't think it's worth writing that code .

> So this ends up addreffing elem again.
> Could we possibly use a hashtable which doesn't use internally
> nsCOMPtr/RefPtr. I don't see reason to use

I'm curious what the end of this sentence was supposed to be. AFAIK nsInterfaceHashtable is the proper Gecko hashtable to use for XPCOM objects. That being said, it's pretty easy to add a version of nsInterfaceHashtable::Put that moves ownership to the hashtable. I think it's worth adding, so I'll do that.

> don't you end up copying the array here.
> (assuming no compiler optimizations are happening)

nsTArray has a move constructor and local variables being returned are automatically moved. So this shouldn't copy the array, instead only moving it into the temporary in the single user.

> So this stuff doesn't enter to shadow DOM at all.

Indeed -- fixing that now.
Attachment #8952905 - Flags: review?(bugs)
Attachment #8953302 - Flags: review?(bugs)
(In reply to Blake Kaplan (:mrbkap) from comment #6)

> nsTArray has a move constructor and local variables being returned are
> automatically moved. So this shouldn't copy the array, instead only moving
> it into the temporary in the single user.
last I heard that was a compiler optimization, not something the language guarantees, but I could be wrong here.
(In reply to Blake Kaplan (:mrbkap) from comment #6)
> I'm curious what the end of this sentence was supposed to be.
Oops, it was supposed to end something like
"to use strong references". AddRef/Releasing is slow.

 AFAIK
> nsInterfaceHashtable is the proper Gecko hashtable to use for XPCOM objects.
If you want to have strong refs sure.
Comment on attachment 8953302 [details]
Bug 1326028 - We now upgrade elements in the proper order.

https://reviewboard.mozilla.org/r/222570/#review228638
Attachment #8953302 - Flags: review?(bugs) → review+
Comment on attachment 8953301 [details]
Bug 1326028 - Allow moving a reference into nsInterfaceHashtable.

https://reviewboard.mozilla.org/r/222568/#review228644
Attachment #8953301 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #10)
> last I heard that was a compiler optimization, not something the language
> guarantees, but I could be wrong here.

Might you be thinking of return value optimization? Move semantics are definitely part of the language.

I don't think I can avoid the single addref/release pair in this code. The initial list of candidates is a list of weak references. In order to get at the actual objects, I need to do_QueryReferent.
Attachment #8952905 - Flags: review?(bugs)
Comment on attachment 8952905 [details]
bug 1326028 - Upgrade elements in the right order.

https://reviewboard.mozilla.org/r/222152/#review228814

::: dom/base/CustomElementRegistry.cpp:486
(Diff revision 2)
> +    mCandidates.Put(key, elem.forget());
> +  }
> +}
> +
> +nsTArray<nsCOMPtr<Element>>
> +CandidateFinder::OrderedCandidates()

outparam usage would be still faster since it would avoid extra ctor/dtor of the nsTArray, at least in case those don't get optimized out.
But I guess this is ok too.

::: dom/base/CustomElementRegistry.cpp:519
(Diff revision 2)
> +    if (mCandidates.Count() == 0) {
> +      return false;
> +    }
> +  }
> +
> +  FlattenedChildIterator it(aRoot);

This does wrong thing when aRoot is a slot element with assigned nodes. Per spec we shouldn't iterate those here, as far as I see.
We should just enter the ShadowRoot, in case we find a host element.
Attachment #8952905 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] (unusual timezone [JST] for a week) from comment #15)
> outparam usage would be still faster since it would avoid extra ctor/dtor of
> the nsTArray, at least in case those don't get optimized out.
> But I guess this is ok too.

I'm going to keep this as it is. If I get a chance, I'll read the output assembly to see how well gcc does with this. Since the nsTArray constructors are all inline, I'd be surprised if this added anything more than a few branches to this path.

> This does wrong thing when aRoot is a slot element with assigned nodes. Per
> spec we shouldn't iterate those here, as far as I see.
> We should just enter the ShadowRoot, in case we find a host element.

My eyes keep bouncing off of the definition of "shadow-including tree order"... This is much simpler than I was making it.
Comment on attachment 8952905 [details]
bug 1326028 - Upgrade elements in the right order.

https://reviewboard.mozilla.org/r/222152/#review231172
Attachment #8952905 - Flags: review?(bugs) → review+
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/414efc66b026
Allow moving a reference into nsInterfaceHashtable. r=smaug
https://hg.mozilla.org/integration/autoland/rev/ab4ee52d5a81
Upgrade elements in the right order. r=smaug
https://hg.mozilla.org/integration/autoland/rev/38057b774238
We now upgrade elements in the proper order. r=smaug
Shadow DOM is disabled if stylo is disabled. I supposed technically we could fix the test to not rely on Shadow DOM, but it's easier to just update the test expectations.
Flags: needinfo?(mrbkap)
Attachment #8953302 - Flags: review+ → review?(bugs)
Comment on attachment 8953302 [details]
Bug 1326028 - We now upgrade elements in the proper order.

https://reviewboard.mozilla.org/r/222570/#review233084

yes, we rely on stylo on all the shadow DOM tests these days.
Attachment #8953302 - Flags: review?(bugs) → review+
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/29aeb9151593
Allow moving a reference into nsInterfaceHashtable. r=smaug
https://hg.mozilla.org/integration/autoland/rev/c5f33612c491
Upgrade elements in the right order. r=smaug
https://hg.mozilla.org/integration/autoland/rev/544d195743e3
We now upgrade elements in the proper order. r=smaug
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: