944011 - Remove explicit cx pushing from event code

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Bobby Holley (:bholley)]

This is probably the most involved usage of manual cx pushing today, because there are a lot of performance optimizations with the RePush stuff. I was talking with bz and smaug on IRC today, and we think that this can actually just go away now that event handlers/listeners are on WebIDL.

Local smoke testing is promising.

Comment 1

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=c3a5756c3ec8

Comment 2

[Bobby Holley (:bholley)]

Looks like exactly one test broke: content/base/test/test_bug372964-2.html

This test makes sure that we don't call event handlers for event targets whose inner is not current. Smaug says this used to be very important to prevent bugs like bug 460002, but is less sure now. He's going to look into this.

Comment 3

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

I think I've convinced myself that the new behavior is ok, well, not worse than the current one.
In the test you'd need to add the listener which is scope of the iframe.
So perhaps load data:text/html,<script>function...</script> as src of it.

This change is rather scary, so landing early in a cycle would be good.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #3)
> I think I've convinced myself that the new behavior is ok, well, not worse
> than the current one.

(In reply to Olli Pettay [:smaug] from comment #3)
> I think I've convinced myself that the new behavior is ok, well, not worse
> than the current one.

Can you fill me in a bit on the reasoning, just so that I understand this better in the future?

You also mentioned that one of the big features of cx pushing was that it did current-inner checking. Can you tell me where/how that happens? I don't see it.

> This change is rather scary, so landing early in a cycle would be good.

Ok, I'll land it next week.

Comment 5

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

See http://mxr.mozilla.org/mozilla-central/source/dom/bindings/CallbackObject.cpp?rev=48e320f58f71#69

Comment 6

[Bobby Holley (:bholley)]

Attachment: Remove explicit cx pushing from event code. v2

Olli approves - lets get Boris' opinion as well. If all goes well I'll land
early next week after the merge.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8343543 [details] [diff] [review]
Remove explicit cx pushing from event code. v2

You're working in an old tree. :( In particular, this needs to merge with http://hg.mozilla.org/mozilla-central/rev/8e1efc26dd88

I'd appreciate you doing the merge before I spend more time reviewing this.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Remove explicit cx pushing from event code. v3

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8344911 [details] [diff] [review]
Remove explicit cx pushing from event code. v3

Are we sure we don't need a cx (and in particular the cx we're compiling on) pushed during compilation?  I can see why we might not, nowadays, but just want to make sure.

r=me if we're happy with that part

Comment 10

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #9)
> Are we sure we don't need a cx (and in particular the cx we're compiling on)
> pushed during compilation?  I can see why we might not, nowadays, but just
> want to make sure.

Hm, you might be right. The one real concern here is the cx that exceptions will be reported on. I'll be safe and replace the AutoJSContext with an AutoPushJSContext at the top of CompileEventHandlerInternal. Carrying over review, let me know if you have a problem with that.

Comment 11

[Bobby Holley (:bholley)]

Attachment: Remove explicit cx pushing from event code. v4 r=bz

Comment 12

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=6d0147b37f6a

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yes, the AutoPushJSContext makes sense to me.

Comment 14

[Bobby Holley (:bholley)]

Ugh, one stupid emulator-only marionette webAPI failure. Added some logging and pushing that:

https://tbpl.mozilla.org/?tree=Try&rev=add57f1c9064

Comment 15

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=cf6f174952c3

Comment 16

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=f885007eff1c

Comment 17

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=52d56f36f9c8

Comment 18

[Bobby Holley (:bholley)]

Ok, so I've finally managed to get some useful logging out of the try push in comment 17. Here's the relevant part:

https://pastebin.mozilla.org/3757808

The test is here: http://mxr.mozilla.org/mozilla-central/source/dom/telephony/test/marionette/test_incoming_answer_hangup_oncallschanged.js

What appears to be happening is that the test successfully runs the code in the |onconnecting| handler, and then everything hangs. My instrumentation in TelephonyCall::ChangeStateInternal shows us doing a bunch of state changes to nsITelephonyProvider::CALL_STATE_HELD. Are those coming from the RIL? Seems like that shouldn't be happening IIUC.

Hsin-Yi, can you have a look and tell me what you think is going on? This patch in this bug makes some deep changes to event dispatch. I'm a bit perplexed as to why the changes are green everywhere, except this one telephony test.

Comment 19

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

(In reply to Bobby Holley (:bholley) from comment #18)
> Ok, so I've finally managed to get some useful logging out of the try push
> in comment 17. Here's the relevant part:
> 
> https://pastebin.mozilla.org/3757808
> 
> The test is here:
> http://mxr.mozilla.org/mozilla-central/source/dom/telephony/test/marionette/
> test_incoming_answer_hangup_oncallschanged.js
> 
> What appears to be happening is that the test successfully runs the code in
> the |onconnecting| handler, and then everything hangs. My instrumentation in
> TelephonyCall::ChangeStateInternal shows us doing a bunch of state changes
> to nsITelephonyProvider::CALL_STATE_HELD. Are those coming from the RIL?
> Seems like that shouldn't be happening IIUC.
> 

You are right, bholley. State changing to nsITelephonyProvider::CALL_STATE_HELD is coming from RIL. This shouldn't be happening in test_incoming_answer_hangup_oncallschanged.js.

> Hsin-Yi, can you have a look and tell me what you think is going on? This
> patch in this bug makes some deep changes to event dispatch. I'm a bit
> perplexed as to why the changes are green everywhere, except this one
> telephony test.

I feel the same as you. I am wondering how your patch could trigger these weird ril state change. I am trying to locally run the test with your patch and with more debug information enabled. Hopefully we get more ideas then.

If you are going to have another try run, in addition to your better logging patch, please also enable ril debug by setting the flag "this.DEBUG_ALL" to true in dom/system/gonk/ril_const.js.

Comment 20

[Bobby Holley (:bholley)]

Pushed again with this.DEBUG_ALL: https://tbpl.mozilla.org/?tree=Try&rev=f87ad1b34932

At this point there's not a ton I can do - hopefully hsinyi can figure it out. :-)

Comment 21

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

(In reply to Bobby Holley (:bholley) from comment #20)
> Pushed again with this.DEBUG_ALL:
> https://tbpl.mozilla.org/?tree=Try&rev=f87ad1b34932
> 
> At this point there's not a ton I can do - hopefully hsinyi can figure it
> out. :-)

Per try full log, test_incoming_answer_hangup.js starts at line #12224 and ends at line #12417. However, even after this test is finished, I could still see the related messages appearing (eg. line #12999) among other tests, such as test_incoming_answer_hangup_oncallschanged.js.

With more investigation, I realized the weird behaviour is caused by the fact that we didn't remove event listener 'telephony.onincoming" in test_incoming_answer_hangup.js. And this patch touching event code somehow triggers the case. 

I will file another bug to fix the test case, then everything should be fine. :)

Comment 22

[Boris Zbarsky [:bzbarsky]]

Presumably that's fallout from the issue described in comment 2?

Comment 23

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #22)
> Presumably that's fallout from the issue described in comment 2?

Yeah. Though note that, in general, we still _do_ perform a current inner check in CallSetup. In comment 2, the issue turned out to be document.open. I don't know enough about Marionette's setup to have a good idea of why that's affecting us here.

Comment 24

[Boris Zbarsky [:bzbarsky]]

In CallSetup we perform a current-inner check on the callee function.

So if you have script in window A that does:

  nodeFromWindowB.onfoo = function() { ... };

then unload window B (but still have window A loaded), the inner of the function will be current, but the inner of the node will not be anymore.

Comment 25

[Bobby Holley (:bholley)]

Ok, I think it makes sense to add a test for this one. I'll whip one up real quick.

Comment 26

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #25)
> Ok, I think it makes sense to add a test for this one. I'll whip one up real
> quick.

And to clarify - is what we're doing here spec-correct? Or is that part underdefined? If it is, we should probably get a bug on file.

Comment 27

[Boris Zbarsky [:bzbarsky]]

I suspect it's not explicitly defined in the spec that stuff should just stop working when you navigate, so the spec effectively says things should keep working, I expect.

smaug would know for sure.

Comment 28

[Bobby Holley (:bholley)]

Attachment: Tests. v2 r=bz

Attaching tests. carrying over review from bug 949949 comment 8.

Comment 29

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=b1c314c612fa

Comment 30

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #27)
> I suspect it's not explicitly defined in the spec that stuff should just
> stop working when you navigate, so the spec effectively says things should
> keep working, I expect.
> 
> smaug would know for sure.

Ok. But the spec _does_ define that the document associated with the callee must be active?

Comment 31

[Boris Zbarsky [:bzbarsky]]

> But the spec _does_ define that the document associated with the callee must be active?

I would expect not.

Comment 32

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #31)
> > But the spec _does_ define that the document associated with the callee must be active?
> 
> I would expect not.

Then we should get a spec bug on file for that, yeah?

I'm happy to file it if you want, but I think you know more about this issue than I do, and would probably generate a more compelling argument.

Comment 33

[Boris Zbarsky [:bzbarsky]]

I do think we want a spec bug.

I also think we need some testing of what different UAs do in some of these situations, possibly before filing the spec bug.

In all cases, I volunteer smaug.  ;)

Comment 34

[Bobby Holley (:bholley)]

try run in comment 29 looks green:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3dc5307c264d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ff8a92caa586

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3dc5307c264d
https://hg.mozilla.org/mozilla-central/rev/ff8a92caa586

Comment 36

[Ioana (away)]

https://hg.mozilla.org/mozilla-central/rev/ff8a92caa586

Comment 37

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

(In reply to Bobby Holley (:bholley) from comment #30)
> Ok. But the spec _does_ define that the document associated with the callee
> must be active?

I'm not aware of such. Things should just work.

Comment 38

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #37)
> (In reply to Bobby Holley (:bholley) from comment #30)
> > Ok. But the spec _does_ define that the document associated with the callee
> > must be active?
> 
> I'm not aware of such. Things should just work.

Boris seems to be suggesting in comment 33 that it should not work, and that we should file a spec bug on it.

Comment 39

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

I still think things should just work. And if things should just work, there isn't a spec bug to file.

Though, I admit that if all the implementations do X and the spec doesn't say anything about it, 
it should be added to the spec.