1422847 - JS Event handler ignored if there is no breakpoint before it's first use

Status: NEW

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

Description

[Martin Häcker]

Attachment: after event should have been invoked.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_1) AppleWebKit/604.3.5 (KHTML, like Gecko) Version/11.0.1 Safari/604.3.5

Steps to reproduce:

I have an element that gets a click event attached via jQuery, then gets rendered, and the gets the click event triggered (also via jQuery) on it.

Now the strange behavior is, that since Firefox 55 this click event is not triggered anymore - unless I have a breakpoint before this invocation. (Firefox 54 and below work fine, as do of course WebKit / Chrome, the problem persists in the current beta and nightly build)

I have absolutely no clue why this happens - and this also doesn't happen with other similar events - just this one.

I haven't managed a reduction yet, but the code looks more or less like this:

					var switcher = $(this.graph.legendElement).find('.group-toggle')
					// debugger
					switcher.click()
					expect(this.series[0].disabled).toBe(true); // fails
					expect(this.series[2].disabled).toBe(true);
					expect(this.series[3].disabled).toBe(false); // fails
					debugger
if the first debugger statement is commented in, the code does what is expected, if it is commented out, the event handler fails and thus it's effect is not registered in the unit test. (Of course the input in question is attached to the DOM as firefox still has problems correctly triggering events on elements that are not attached to the dom).

The Event handler is attached with code like this:
				debugger
				this.$el.find('.group-toggle').click(function(ev){
					debugger
					toggleGroupBreakdown(ev);
					applySettings();
				});
the debugger statement inside the event handler is _not_ reached if the first debugger statement in the first statement is not commented in.

I've also attached a screenshot that documents at the second breakpoint in the first snippet, that firefox actually knows that there is a click event handler.

Not sure how to proceed - minimizing this would be _quite_ time consuming, as I have no clue where to start, so I want to get some feedback from you guys first.


Actual results:

The click event handler is not triggered - unless I put a breakpoint before it.


Expected results:

The click event handler should be triggered.

Comment 1

[Martin Häcker]

If you want to view this test failing, go to https://actest.devc7.yeepa.de/testing/js/?spec=yeepa.index%20IndexGraph%20IndexGraphContentControls%3A%20per-topic%2Fgroup%20switching%20should%20activate%20series%20the%20user%20selected.

If you want to try the breakpoint behavior, try setting a breakpoint at yeepa-index-graph-spec.js:334

(If you stop there, another error will show up, because somehow stopping in the debugger pollutes the global namespace - but that is another issue).

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

I don't know why this was put in the console, but anyway, I looked at it.
For me, it looks like a race condition in your code.
if you put a debugger between you click instruction and your assertion, it may give a little more time to complete what it should do (and make your tests pass).

Could you transform you function into an async one, and use `await new Promise(res => setTimeout(res, 1000))` in place of your debugger statements ? This will make the code wait for 1 second.
If your tests pass, then it's because you have a race.

Comment 3

[Robert Buchholz]

It seems that unit test does not trigger any XHR, uses no background workers and only interacts with the DOM. What kind of racy APIs do you have in mind the code could be using (it was running reliably in FF55, is running reliably in Chrome and breaks reliably in FF57)?

Comment 4

[Martin Häcker]

I'd like to add that I modified the code in question like this:

```
					var switcher = $(this.graph.legendElement).find('.group-toggle')
					// debugger
					console.log($._data(switcher[0], 'events').click)
					switcher.click()
					expect(this.series[0].disabled).toBe(true); // fails
					expect(this.series[2].disabled).toBe(true);
					expect(this.series[3].disabled).toBe(false); // fails
					console.log($._data(switcher[0], 'events').click)
					// debugger
```

So I can see that the click handler is attached successfully (as far as jQuery is concerned) before the click event is triggered. Still it does not fire. (Thinking about this: this makes no sense at all, as jQuery should not even go through the native event handling machinery of the browser, but invoke it's own handlers directly.

Are there any asynchronous apis that I'm not aware of that Firefox has added since Release 54 that could change the behavior of APIs like JQuery to become asynchronous in such core parts of the DOM interaction?

Indeed making the test asynchronous, in that it waits for a second before trying to click that button makes it pass (even when waiting just 0 milliseconds - but it still doesn't explain at all why that would be neccessary, us using just synchronous APIs (to the best of our knowledge).

I have to admit, that this indeed looks quite like a race condition in the FF Dom implementation to me - still I know that this is quite unlikely.

Comment 5

[Martin Häcker]

Out of curiosity: Wouldn't this better be classified in the JS engine or the dom engine?

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

it should, but i want to narrow this down.
would you be able to put a contrived failing example somewhere (as an html file here, or on codpen or glitch), so we can really see what's going on.

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

i tried to hunt this but this is complex with jasmine doing its job.
could you provide an access to the app so i can run the same script you do in jasmine and see what's happening ?

Comment 8

[Martin Häcker]

:-/ Sorry, the app is not open source, so I cannot provide the full thing. The staging server is accessible, and of course you can set breakpoints / try stuff with the console. Which is not much I'm sad to say. I can of course provide you with an account - but I guess that is not what you meant.

As for a reduction: I may be able to try something tomorrow, but if this is indeed a race condition somewhere (and it looks that way) reducing this will be quite hard. Especially because the exact same code path works fine in adjacent tests.

Comment 9

[Martin Häcker]

Just as a fun exercise, I tried mozregression to see if it comes up with something useful. What a fun tool! :-)

Here's the revision it came up with as being the first bad one. Does that make any sense to you? At least it seems to be on topic as it does something with click handlers.

```
11:35.06 INFO: No more inbound revisions, bisection finished.
11:35.06 INFO: Last good revision: 7dee6ff041fe99439e28aefa39c7ded8d3f530a3
11:35.06 INFO: First bad revision: bec20d74d93a82d584804f5cce6019d23db57666
11:35.06 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7dee6ff041fe99439e28aefa39c7ded8d3f530a3&tochange=bec20d74d93a82d584804f5cce6019d23db57666
```

I have attached my full mozregression log.

Comment 10

[Martin Häcker]

Attachment: Full mozregression log

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Thanks Martin for tracking the regression

Comment 12

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

Olli, any thougths about this regression?

Comment 13

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

Late with this.
Reporter, could you perhaps provide a testcase here, attached to the bug?
Hopefully even without jquery. Or is this a bug in jquery perhaps, does it have some Gecko specific code here?


And what is "firefox still has problems correctly triggering events on elements that are not attached to the dom" about? Event dispatch doesn't really care about whether element is in DOM or not, but perhaps you're talking about some specific case here?

Comment 14

[Martin Häcker]

@Olli: The Testcase is still live (to the best of my knowledge) https://actest.devc7.yeepa.de/testing/js/?spec=yeepa.index%20IndexGraph%20IndexGraphContentControls%3A%20per-topic%2Fgroup%20switching%20should%20activate%20series%20the%20user%20selected

I haven't been able to reduce this at all, since it seems to be highly timing sensitive, and I'm not sure why this happens. The Testcase however should be easy to follow.

Sadly I have no Idea if this happens in or because jQuery.

As for "firefox still has problems correctly triggering events on elements that are not attached to the dom", that is this issue: https://bugzilla.mozilla.org/show_bug.cgi?id=977034

Comment 15

[Martin Häcker]

@Olli: Any other info I can give you? I already reduced this down to the exact change that triggers this behavior - should this not help greatly in figuring out why this happens?

Comment 16

[Martin Häcker]

@Olli: ping? Any progress on this?

Comment 17

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

Sorry, where is the minimal testcase? Could you attach a minimal testcase to this bug, pretty please.

When I load https://actest.devc7.yeepa.de I just see a page loaded 
and it saying
1 spec, 0 failures in 13.576s

Comment 18

[Robert Buchholz]

I can confirm the link from comment #14 does not show a test failure in Nightly 61, but I see it in the 59 release channel.

Comment 19

[Martin Häcker]

Also cannot reproduce anymore with Developer Edition 60.0b11 (64-Bit) - seems it got fixed. Congrats!

Do you see any commits in the area affected by the commit I bisected this down to? Was there a duplicate of this bug raised somewhere?

Comment 20

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

I'm not aware of anything related. Might be worth to look for un-regression-range.

Comment 21

[Martin Häcker]

According to mozregression this commit fixed it: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=977c4997720247488f97bc9a9e9111a5bb173685&tochange=03d139086ace13575d00863456134a010751ad97

```
10:42.63 INFO: Narrowed inbound regression window from [ac305af3, 03d13908] (3 builds) to [977c4997, 03d13908] (2 builds) (~1 steps left)
10:42.63 INFO: No more inbound revisions, bisection finished.
10:42.63 INFO: First good revision: 03d139086ace13575d00863456134a010751ad97
10:42.63 INFO: Last bad revision: 977c4997720247488f97bc9a9e9111a5bb173685
10:42.63 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=977c4997720247488f97bc9a9e9111a5bb173685&tochange=03d139086ace13575d00863456134a010751ad97
```

Does that make sense to you?

Comment 22

[Martin Häcker]

Attachment: Full mozregression log for the un-regression

How I found the unregression

Comment 23

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

Surprising regression range.

Comment 24

[Martin Häcker]

Could you perhaps elaborate a bit? (Mozilla Source Code is still a big mystery to me)

Comment 25

[Martin Häcker]

To be more clear: as far as I can figure out, the two commits don't seem to even work on the same code, i.e. maybe there is still a bug lurking that was triggered and hidden by these two commits?

Comment 26

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

I mean, as you say, that un-regression range didn't change any relevant code, which is why the un-regression range is surprising.

Comment 27

[Martin Häcker]

This sounds like you don't plan to investigate why this was caused and fixed by these commits. :-)

(While I can understand that, it still seems like there is a bug somewhere, that is probably masked by other behavior.)