Remove ondialogaccept and ondialogcancel attributes from xul files
Categories
(Core :: DOM: Security, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox68 | --- | fixed |
People
(Reporter: jallmann, Assigned: jallmann)
References
(Blocks 1 open bug)
Details
(Whiteboard: [domsecurity-active])
Attachments
(1 file, 1 obsolete file)
(deleted),
text/x-phabricator-request
|
Details |
+++ 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.
[2] - https://searchfox.org/mozilla-central/search?q=ondialogaccept&path=
[3] - https://searchfox.org/mozilla-central/search?q=ondialogcancel&path=
Assignee | ||
Comment 1•6 years ago
|
||
Removed all occurences of ondialogaccept.
Removed all occurences of ondialogcancel.
Replaced all removed attributes with event handlers.
Assignee | ||
Comment 2•6 years ago
|
||
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
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•6 years ago
|
||
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 attributesondialogaccept="return dialog.onOK()"
andondialogcancel="return dialog.onCancel()"
inAs far as I understand, the responsible script is
HelperAppDlg.jsm
and I'm trying to add the eventHandlers somewhere inhttps://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/HelperAppDlg.jsm#411
Yep.
I don't know how to correctly reference the functions
onOK()
andonCancel()
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 thethis.mDialog.document.addEventListener
call is. If you want to preserve the samethis
, 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 toremoveEventListener
that will remove the listener again). - passing
this.onOK
will work but have a differentthis
when called. Again, an arrow function could work, or you could passthis.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 noonOK
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!
Updated•6 years ago
|
Comment 4•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Comment 5•6 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.
Updated•6 years ago
|
Assignee | ||
Comment 6•6 years ago
|
||
Comment 8•6 years ago
|
||
bugherder |
Assignee | ||
Updated•6 years ago
|
Updated•6 years ago
|
Comment 9•6 years ago
|
||
It would be nice if you could CC someone from the Thunderbird team on bugs like this one.
Comment 10•6 years ago
|
||
(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•6 years ago
|
||
What's the meta-bug? The eval stuff in bug 1473549? I thought this is part of the de-XUL effort, no?
Comment 12•6 years ago
|
||
(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.
Description
•