Closed Bug 1498564 Opened 6 years ago Closed 6 years ago

Remove new Function from places tree.xml

Categories

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

60 Branch
enhancement

Tracking

()

RESOLVED FIXED
Firefox 67
Tracking Status
firefox67 --- fixed

People

(Reporter: vinoth, Assigned: jallmann)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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)

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

Flags: needinfo?(cegvinoth)
Assignee: nobody → jallmann
Flags: needinfo?(cegvinoth)

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

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

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: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: