Remove new Function from places tree.xml

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P3
normal
RESOLVED FIXED
9 months ago
5 months ago

People

(Reporter: vinoth, Assigned: jallmann)

Tracking

(Blocks 1 bug)

60 Branch
Firefox 67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment)

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
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
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 3

6 months ago

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

Updated

6 months ago
Flags: needinfo?(cegvinoth)
Reporter

Updated

5 months ago
Assignee: nobody → jallmann
Flags: needinfo?(cegvinoth)
Assignee

Comment 4

5 months ago

(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?

Flags: needinfo?(cegvinoth)

(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.

Flags: needinfo?(cegvinoth)
Assignee

Comment 6

5 months ago

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

Assignee

Comment 7

5 months ago

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.

Flags: needinfo?(gijskruitbosch+bugs)

(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. :-)

Flags: needinfo?(gijskruitbosch+bugs)

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.

Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
You need to log in before you can comment on or make changes to this bug.