Closed
Bug 944011
Opened 11 years ago
Closed 10 years ago
Remove explicit cx pushing from event code
Categories
(Core :: DOM: Events, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(2 files, 2 obsolete files)
22.41 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
2.55 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=c3a5756c3ec8
Assignee | ||
Comment 2•11 years ago
|
||
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.
Flags: needinfo?(bugs)
Comment 3•10 years ago
|
||
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.
Flags: needinfo?(bugs)
Assignee | ||
Comment 4•10 years ago
|
||
(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•10 years ago
|
||
See http://mxr.mozilla.org/mozilla-central/source/dom/bindings/CallbackObject.cpp?rev=48e320f58f71#69
Assignee | ||
Comment 6•10 years ago
|
||
Olli approves - lets get Boris' opinion as well. If all goes well I'll land early next week after the merge.
Attachment #8343543 -
Flags: review?(bzbarsky)
Comment 7•10 years ago
|
||
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.
Attachment #8343543 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8343543 -
Attachment is obsolete: true
Attachment #8344911 -
Flags: review?(bzbarsky)
Comment 9•10 years ago
|
||
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
Attachment #8344911 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 10•10 years ago
|
||
(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.
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8344911 -
Attachment is obsolete: true
Attachment #8345338 -
Flags: review+
Assignee | ||
Comment 12•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=6d0147b37f6a
Comment 13•10 years ago
|
||
Yes, the AutoPushJSContext makes sense to me.
Assignee | ||
Comment 14•10 years ago
|
||
Ugh, one stupid emulator-only marionette webAPI failure. Added some logging and pushing that: https://tbpl.mozilla.org/?tree=Try&rev=add57f1c9064
Assignee | ||
Comment 15•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=cf6f174952c3
Assignee | ||
Comment 16•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=f885007eff1c
Assignee | ||
Comment 17•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=52d56f36f9c8
Assignee | ||
Comment 18•10 years ago
|
||
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.
Flags: needinfo?(htsai)
Comment 19•10 years ago
|
||
(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.
Flags: needinfo?(htsai)
Assignee | ||
Comment 20•10 years ago
|
||
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. :-)
Flags: needinfo?(htsai)
Comment 21•10 years ago
|
||
(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. :)
Flags: needinfo?(htsai)
Comment 22•10 years ago
|
||
Presumably that's fallout from the issue described in comment 2?
Assignee | ||
Comment 23•10 years ago
|
||
(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•10 years ago
|
||
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.
Assignee | ||
Comment 25•10 years ago
|
||
Ok, I think it makes sense to add a test for this one. I'll whip one up real quick.
Assignee | ||
Comment 26•10 years ago
|
||
(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•10 years ago
|
||
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.
Assignee | ||
Comment 28•10 years ago
|
||
Attaching tests. carrying over review from bug 949949 comment 8.
Attachment #8347351 -
Flags: review+
Assignee | ||
Comment 29•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b1c314c612fa
Assignee | ||
Comment 30•10 years ago
|
||
(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?
Flags: needinfo?(bugs)
Comment 31•10 years ago
|
||
> But the spec _does_ define that the document associated with the callee must be active?
I would expect not.
Assignee | ||
Comment 32•10 years ago
|
||
(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•10 years ago
|
||
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. ;)
Assignee | ||
Comment 34•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3dc5307c264d https://hg.mozilla.org/mozilla-central/rev/ff8a92caa586
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 36•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ff8a92caa586
Flags: in-testsuite+
Comment 37•10 years ago
|
||
(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.
Flags: needinfo?(bugs)
Assignee | ||
Comment 38•10 years ago
|
||
(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.
Flags: needinfo?(bugs)
Comment 39•10 years ago
|
||
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.
Flags: needinfo?(bugs)
You need to log in
before you can comment on or make changes to this bug.
Description
•