1529231 - Remove ondialogaccept and ondialogcancel attributes from xul files

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P3)

Description

[Jonas Allmann [:jallmann]]

+++ This bug was initially created as a clone of Bug #1525636 +++

In order to remove the usage of new Function from dialog.xml[1], all occurences of the ondialogaccept[2] and ondialogcancel[3] attributes need to be removed from the codebase and replaced with event handlers.

[1] - https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/toolkit/content/widgets/dialog.xml#418

[2] - https://searchfox.org/mozilla-central/search?q=ondialogaccept&path=

[3] - https://searchfox.org/mozilla-central/search?q=ondialogcancel&path=

Comment 1

[Jonas Allmann [:jallmann]]

Attachment: Bug 1529231, Remove all occurences of ondialogaccept and ondialogcancel, r=gijs

Removed all occurences of ondialogaccept.
Removed all occurences of ondialogcancel.
Replaced all removed attributes with event handlers.

Comment 2

[Jonas Allmann [:jallmann]]

I'm having difficulties with the following files:
I want to replace the two attributes ondialogaccept="return dialog.onOK()" and ondialogcancel="return dialog.onCancel()" in

https://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/content/unknownContentType.xul#30

As far as I understand, the responsible script is HelperAppDlg.jsm and I'm trying to add the eventHandlers somewhere in

https://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/HelperAppDlg.jsm#411

I don't know how to correctly reference the functions onOK() and onCancel() in the definition of the event handlers.
Things I've tried include the following:

this.mDialog.document.addEventListener("dialogaccept", function() { dialog.onOK(); });
this.mDialog.document.addEventListener("dialogaccept", function() { nsUnknownContentTypeDialog.onOK(); });
this.mDialog.document.addEventListener("dialogaccept", function() { this.onOK(); });
this.mDialog.document.addEventListener("dialogaccept", function() { this.mDialog.dialog.onOK(); });
this.mDialog.document.addEventListener("dialogaccept", this.onOK);
this.mDialog.document.addEventListener("dialogaccept", onOK);

None of these appear to work. Either the function can not be referenced at all or subsequent references inside onOK() are broken. I'm out of ideas how this could work and would be very thankful for advice.

Comment 3

[:Gijs (he/him)]

Sorry for the delay in response.

(In reply to Jonas Allmann [:jallmann] from comment #2)

I'm having difficulties with the following files:
I want to replace the two attributes ondialogaccept="return dialog.onOK()" and ondialogcancel="return dialog.onCancel()" in

https://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/content/unknownContentType.xul#30

As far as I understand, the responsible script is HelperAppDlg.jsm and I'm trying to add the eventHandlers somewhere in

https://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/HelperAppDlg.jsm#411

Yep.

I don't know how to correctly reference the functions onOK() and onCancel() in the definition of the event handlers.
Things I've tried include the following:

this.mDialog.document.addEventListener("dialogaccept", function() { dialog.onOK(); });
this.mDialog.document.addEventListener("dialogaccept", function() { nsUnknownContentTypeDialog.onOK(); });
this.mDialog.document.addEventListener("dialogaccept", function() { this.onOK(); });
this.mDialog.document.addEventListener("dialogaccept", function() { this.mDialog.dialog.onOK(); });
this.mDialog.document.addEventListener("dialogaccept", this.onOK);
this.mDialog.document.addEventListener("dialogaccept", onOK);

None of these appear to work. Either the function can not be referenced at all or subsequent references inside onOK() are broken. I'm out of ideas how this could work and would be very thankful for advice.

So the reason these don't work is:

• the first two: they're being run in the .jsm scope, and there is no global "dialog" variable. "nsUnknownContentTypeDialog" is a function (used as a constructor) and it doesn't have an "onOK" property; only the instances constructed using that function would do. We need to reference the instance we're using for this dialog.
• the ones with a function() closure that use 'this' - the event handler won't get invoked with the same this as the method that's adding the event listeners (ie the place where the this.mDialog.document.addEventListener call is. If you want to preserve the same this, you could use an arrow function. However, passing a function instance inline like that would make it impossible to remove the listener (because we're not keeping a reference to the (arrow) function we're passing, and so there's nothing we can pass to removeEventListener that will remove the listener again).
• passing this.onOK will work but have a different this when called. Again, an arrow function could work, or you could pass this.onOK.bind(this). In some cases, we re-bind methods on objects in the constructor so that this type of passing of function references works, and we can then also remove the listener again using that reference. See e.g. https://searchfox.org/mozilla-central/source/browser/base/content/browser-pageActions.js#51 for an example.
• passing onOK literally doesn't work because there's no onOK variable in the global scope in the jsm.

Do we ever remove the listeners? We might need to do so to avoid leaks...

I'd suggest something like this:

this.mDialog.document.addEventListener("dialogaccept", this);
this.mDialog.document.addEventListener("dialogcancel", this);

and then define a handleEvent method on nsUnknownContentTypeDialog.prototype. This will correctly keep this as the same nsUnknownContentTypeDialog instance, and so you can call this.onOK() and this.onCancel() or whatever from that method, based on the event type (handleEvent will be passed 1 argument, which is the event).

You can then factor out the final few lines of onOK and onCancel, which are the same, into an onUnload method, and there also remove the listeners again (removeEventListener("dialogaccept", this) etc.).

Hopefully that helps!

Comment 4

[:Gijs (he/him)]

This is approved and ready to land but can't because it conflicts with the fix from bug 1535080 - can you rebase this patch on top of that one, and then resubmit to phabricator? (using moz-phab and hg, moz-phab submit . will submit only the current commit, which should help)

Comment 5

[Natalia Csoregi [:nataliaCs]]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

Comment 6

[Jonas Allmann [:jallmann]]

Attachment: Bug 1529231, Remove all occurences of ondialogaccept and ondialogcancel, r=gijs

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8691a4583cae
Remove all occurences of ondialogaccept and ondialogcancel, r=Gijs

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8691a4583cae

Comment 9

[Jorg K (CEST = GMT+2)]

It would be nice if you could CC someone from the Thunderbird team on bugs like this one.

Comment 10

[:Gijs (he/him)]

(In reply to Jorg K (GMT+2) from comment #9)

It would be nice if you could CC someone from the Thunderbird team on bugs like this one.

Sorry this took you by surprise; It looks like there's people CC'd on the metabug, and so I assume there's not much point CC'ing you on all the other (mostly closed) items in its dep tree at this point...

Comment 11

[Jorg K (CEST = GMT+2)]

What's the meta-bug? The eval stuff in bug 1473549? I thought this is part of the de-XUL effort, no?

Comment 12

[:Gijs (he/him)]

(In reply to Jorg K (GMT+2) from comment #11)

What's the meta-bug? The eval stuff in bug 1473549?

Yes.

I thought this is part of the de-XUL effort, no?

No.