1551425 - Javascript inconsistancy changing select options textContent

Status: RESOLVED FIXED

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

Description

[Siltaer]

Attachment: bug select textContent.zip

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

I attached 3 JavaScript callbacks to a select box in a form.
The small script changes the textContent of each select option when the select is focused (longer text) and when it looses the focus (shorter text), and changing the selection cause it loosing the focus.

Actual results:

It works for 2 or 3 times and then stops :

• The select have short texts at startup (i.e. phone prefix).
• I click to open it (and the texts become the long version i.e. country name + phone prefix)
• I select an option the focus goes to the other input, and the text shorten (to phone prefix only)

Repeat the process 3 times it will stop working.
But if I set some console.log (as in the 2nd provided script), it works as expected forever.

Expected results:

Should have worked as expected without the console.log.

The problem impacts other versions of Firefox such as TOR Browser.
But Chrome works (damn fast) as expected even without console.log.

Provided a minimal case with only one select option and the minimal required HTML around + two versions of the script (working with console.log, not working without the console.log).

Comment 1

[Liviu Seplecan]

Hi @Siltaar, here is the results of testing this issue:
[Platform affected]: Windows 10, Mac OS X
[Firefox versions affected]: release 66.0.5, beta 67.0b19 and latest nightly 68.0a1

• it can be seen in a screenshot attached -

Additionally, this issue won't occur in Ubuntu 18.04
I will set a component to it and if this isn't a proper one please fell free to change it.
Thanks for your contribution.

Comment 2

[Liviu Seplecan]

Attachment: bug 1551425- select textContent-min.jpg

Comment 3

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

Mirko can help tell us what's up here. :)

Comment 4

[Mirko Brodesser (:mbrodesser-Igalia), offline until 8th of October]

Tried reproducing the issue with the STR posted in the description.

Without logging to console, the problem doesn't seem related to the number of successful runs, but just to timing. When one waits around 10 seconds, it won't work at all.

With logging to console the behavior is as expected. Interestingly, the console doesn't have to be opened.

Comment 5

[Siltaer]

Looks like my variables are getting garbage collected too soon…

Comment 6

[Mirko Brodesser (:mbrodesser-Igalia), offline until 8th of October]

@Siltaar: is that just a suspicion or did you perform some analysis which lead to that conclusion? Please share all findings :)

Comment 7

[Siltaer]

"Looks like", its just an idea.

Comment 8

[Mirko Brodesser (:mbrodesser-Igalia), offline until 8th of October]

@Siltaar: thanks.

I've played more around with the example, and the elements indeed seem to be GC'ed to early.
When adding conditional logging

                          if (local_prefix.state) {
                            console.log(local_prefix.short);
                          } else {
                            console.log(local_prefix.long);
                          }

instead of the unconditional logging, the Web Console shows that local_prefix.long indeed becomes a empty NodeList.

Interestingly, the GC also removes the objects when prefix's and local_prefix's scope is changed to global.

Needs further investigation.

Comment 9

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

The NodeList is from cloned select element and the issue happens after cloned select element get GC'ed which also clears it's childNodes, https://searchfox.org/mozilla-central/rev/aba472751e24763d0c18bae8408e9d7106e9acea/dom/base/nsINode.cpp#125,140. And it looks like the console.log calls prevents cloned element being GC'ed, so we don't encounter the issue.

Comment 10

[Mirko Brodesser (:mbrodesser-Igalia), offline until 8th of October]

(In reply to Edgar Chen [:edgar] from comment #9)

The NodeList is from cloned select element and the issue happens after cloned select element get GC'ed which also clears it's childNodes, https://searchfox.org/mozilla-central/rev/aba472751e24763d0c18bae8408e9d7106e9acea/dom/base/nsINode.cpp#125,140. And it looks like the console.log calls prevents cloned element being GC'ed, so we don't encounter the issue.

Just logging/using the textContent childNode doesn't suffice to prevent the GC from deleting the nodes:

                                console.log('long', local_prefix.long[1].textContent);
                                console.log('short',local_prefix.short[1].textContent);

Whereas logging/using the nodes seems to suffice:

                                console.log('long', local_prefix.long[1]);
                                console.log('short',local_prefix.short[1]);

Comment 11

[Mirko Brodesser (:mbrodesser-Igalia), offline until 8th of October]

Attachment: simpler_test.html

Comment 12

[Mirko Brodesser (:mbrodesser-Igalia), offline until 8th of October]

Attachment: simpler_test.js

Comment 13

[Mirko Brodesser (:mbrodesser-Igalia), offline until 8th of October]

Attachment: Bug 1551425: Part 1) Test reference to childNodes' NodeList keeps its items alive.

Comment 14

[Mirko Brodesser (:mbrodesser-Igalia), offline until 8th of October]

Attachment: Bug 1551425: Part 2) Change reference to parent in `nsAttrChildContentList` from non-owning to RefPtr.

With a non-owning reference, a JS reference to the NodeList didn't keep its
items alive. With this change, the NodeList keeps the parent node (which keeps
its children alive) alive.

Depends on D32836

Comment 15

[Mirko Brodesser (:mbrodesser-Igalia), offline until 8th of October]

Requesting checkin. A corresponding try run has only unrelated failures.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a4f89580ce40
Part 1) Test reference to childNodes' NodeList keeps its items alive. r=smaug
https://hg.mozilla.org/integration/autoland/rev/5a5508254aac
Part 2) Change reference to parent in nsAttrChildContentList from non-owning to RefPtr. r=smaug

Comment 17

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/a4f89580ce40
https://hg.mozilla.org/mozilla-central/rev/5a5508254aac