Closed Bug 1551425 Opened 2 years ago Closed 2 years ago

Javascript inconsistancy changing select options textContent

Categories

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

68 Branch
Desktop
All
defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- fixed

People

(Reporter: sdescarpentries, Assigned: mbrodesser)

Details

Attachments

(6 files)

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

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.

Status: UNCONFIRMED → NEW
Component: Untriaged → JavaScript Engine
Ever confirmed: true
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → Desktop
Component: JavaScript Engine → DOM: Core & HTML

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

Flags: needinfo?(mbrodesser)

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.

Flags: needinfo?(mbrodesser)

Looks like my variables are getting garbage collected too soon…

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

Flags: needinfo?(sdescarpentries)

"Looks like", its just an idea.

Flags: needinfo?(sdescarpentries)

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

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.

Priority: -- → P2

(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]);
Attached file simpler_test.html
Attached file simpler_test.js
Assignee: nobody → mbrodesser
Attachment #9068044 - Attachment description: Bug 1551425: Part 1) Test reference to childNodes NodeList keeps its items alive. → Bug 1551425: Part 1) Test reference to childNodes' NodeList keeps its items alive.

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

Keywords: checkin-needed

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

Pushed by apavel@mozilla.com:
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

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.