Closed Bug 1547718 Opened 6 months ago Closed 3 months ago

Remove `new Function` from preferencesBindings.js

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: jallmann, Assigned: jallmann)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-backlog1])

Attachments

(1 file)

Status: NEW → ASSIGNED

I would like to eliminate these three new Functions, which are used to evaluate event handling code from XUL-attributes, namely:

onbeforeaccept
onsyncfrompreference
onsynctopreference

The first is easily removed and replaced by JS-eventListeners, but the other two look more complicated.
In both cases, the code invoked upon the respective events returns values other than true/false that are processed further. I don't see an immediate way to replicate this behaviour with eventListeners.
What could be a reasonable approach here?

https://searchfox.org/mozilla-central/source/toolkit/content/preferencesBindings.js#345

https://searchfox.org/mozilla-central/source/toolkit/content/preferencesBindings.js#403

Flags: needinfo?(gijskruitbosch+bugs)

Defining a "manual" JS callback/observer infrastructure in preferencesBinding.js that JS on the page can add callbacks/observers to that behave how we want (ie we use returned values if not undefined, or something) seems like the most straightforward way. There's no real need to actually use custom events.

Flags: needinfo?(gijskruitbosch+bugs)

Work in progress.
Add callback infrastructure to preferenceBindings to replace XUL-attributes.

Jorg, heads-up that we're killing onsync{to,from}preference here. I expect thunderbird might care... I also don't think this will land in soft freeze, so probably early (ish?) in the 70 cycle.

Flags: needinfo?(jorgk)

Thanks for the heads-up, forwarding ...

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(jorgk)
Flags: needinfo?(geoff)
Blocks: 1562560

Filed bug 1562560 for thunderbird.

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(geoff)
Keywords: checkin-needed

Umm, what about comment #4. Not landing during soft freeze, which is now. I'll remove the checkin-needed again, no offence intended.

Keywords: checkin-needed

Right, sorry. I missed that, no offence taken.

Jonas, this needs a rebase now (probably due to the prettier changes). Can you rebase and upload the updated version? Sorry for the churn...

Flags: needinfo?(jallmann)

I rebased the patch. Should I flag checkin-needed now?

Flags: needinfo?(jallmann) → needinfo?(gijskruitbosch+bugs)

Sure, sorry, for the extra hassle.

Keywords: checkin-needed
Flags: needinfo?(gijskruitbosch+bugs)

Pushed by opoprus@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d426669c4cba
Remove preferenceBindings.js from eval()-whitelist, r=Gijs

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
You need to log in before you can comment on or make changes to this bug.