1498564 - Remove new Function from places tree.xml

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement, P3)

Description

[Vinothkumar Nagasayanan [:vinoth]]

Eval(), new Function() should never execute with system principal.It is being removed everywhere from our codebase as part of Bug 1473549.

The affected code which should be rewritten,
https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/browser/components/places/content/tree.xml#115

Comment 1

[Vinothkumar Nagasayanan [:vinoth]]

In order to clarify the things I will summarize the changes required for this bug,

In order to remove the usage of new Function from tree.xml[1], 'onopenflatcontainer' attribute present in 'places.xul'[2] should be removed and all related code needs to be rewritten. I am working[3] on adding a custom event handler instead of using attribute in xul files. I will send the patch for review after completing it.
Please correct if I got something wrong about this bug.


[1] - https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/browser/components/places/content/tree.xml#115

[2] - https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/browser/components/places/content/places.xul#392

[3] - https://phabricator.services.mozilla.com/D12257

Comment 2

[:Gijs (he/him)]

(In reply to Vinothkumar Nagasayanan [:vinoth] from comment #1)
> In order to clarify the things I will summarize the changes required for
> this bug,
> 
> In order to remove the usage of new Function from tree.xml[1],
> 'onopenflatcontainer' attribute present in 'places.xul'[2] should be removed
> and all related code needs to be rewritten.

Yep, this seems correct to me.

Comment 3

[komalbharadiya]

Hey, Can I work on this issue? If at all, how do I start?

Comment 4

[Jonas Allmann [:jallmann]]

(In reply to Vinothkumar Nagasayanan [:vinoth] from comment #1)

I am working[3] on adding a
custom event handler instead of using attribute in xul files. I will send
the patch for review after completing it.

[3] - https://phabricator.services.mozilla.com/D12257

Hi Vinoth, am I right that this patch is partly finished and I could use it as a starting point to fix this bug? If yes, could you give me a short hint on what is still missing / not working?

Comment 5

[Vinothkumar Nagasayanan [:vinoth]]

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

(In reply to Vinothkumar Nagasayanan [:vinoth] from comment #1)

I am working[3] on adding a
custom event handler instead of using attribute in xul files. I will send
the patch for review after completing it.

[3] - https://phabricator.services.mozilla.com/D12257

Hi Vinoth, am I right that this patch is partly finished and I could use it as a starting point to fix this bug? If yes, could you give me a short hint on what is still missing / not working?

Yes this patch is only partly complete. I tried to fire a custom event inside treeView.js, which will run the function inside places.js.
But because of some issue with this patch, this custom event is not triggered. You can use this patch or start your own approach from scratch.
You can take a look at a similar Bug 1521040 for reference. In Bug 1521040 we didn't remove the new Function for now, but in this bug you need to create event listener and remove the new Function from tree.xml.

Comment 6

[Jonas Allmann [:jallmann]]

Attachment: Bug 1498564 - Remove new Function from tree.xml and add CustomEvent, r=Gijs

Removed occurence of (new Function) in tree.xml as part of Bug 1473549. Adjusted affected code by adding custom event handler.

Comment 7

[Jonas Allmann [:jallmann]]

Thanks for the review, I applied the small change you suggested.

I haven't yet passed this patch by the try server, which would be recommended before landing, I guess?
Could you give me a hint on what would be a reasonable try push for this?
It's one of my very first patches in mozilla-central, and I have only ever interacted with the nss-try repository, so I'm not familiar yet with try for mozilla central.

Comment 8

[:Gijs (he/him)]

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

Thanks for the review, I applied the small change you suggested.

I haven't yet passed this patch by the try server, which would be recommended before landing, I guess?
Could you give me a hint on what would be a reasonable try push for this?
It's one of my very first patches in mozilla-central, and I have only ever interacted with the nss-try repository, so I'm not familiar yet with try for mozilla central.

Well, the only files you're modifying are in browser/, which indicates only desktop platforms will be affected. Because you're not modifying any gecko internals, you don't really need crashtest/reftest/webplatform-test/mochitest-plain. Because this is UI-only code, there's no point running xpcshell. Because it's JS/frontend-only, you can run just artifact builds (instead of compiling anything). Really, the only things you'd need to run are desktop mochitest-browser-chrome (e10s / non-e10s), mochitest-chrome, mochitest-clipboard (which is basically mochitests that require clipboard access, which are a bit finnicky) and marionette, marionette-e10s and firefox-ui-functional (the python-based functional tests).

You can achieve this using ./mach try fuzzy --artifact and selecting the tests you need, or you can use something like:

./mach try -b od -p win64,macosx64,linux64,linux64-asan -t none -u mochitest-bc,mochitest-clipboard,mochitest-chrome,marionette,marionette-e10s,firefox-ui-functional --artifact

(The e10s version of mochitest-bc should get run automatically without having to specify it.)

Note that I commented on phab that you haven't quite applied the nit that I asked for. :-)

Comment 9

[:Gijs (he/him)]

Oh, and there's no real point running the pgo tests when checkign ./mach try fuzzy - opt and dbg should be fine. If the syntax confuses you (I have no idea how nss-try works and if it's similar or not), https://mozilla-releng.net/trychooser/ can help demystify some of it.

Comment 10

[:Gijs (he/him)]

https://hg.mozilla.org/integration/autoland/rev/bb7602ef747f

pushbot seems to be broken atm?

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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