Closed Bug 1521919 Opened 5 years ago Closed 5 years ago

SitePermissions.get should check for nsIURI

Categories

(Firefox :: Site Identity, enhancement, P3)

Product:
Firefox
Component:
Site Identity
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 68
Tracking Flags:
Tracking Status
firefox68 --- fixed

People

(Reporter: daleharvey, Assigned: phoenixgyaan)

References

Details

Attachments

(1 file)

Bug 1521919 - SitePermissions.get should check for nsIURI. r=johannh
47 bytes, text/x-phabricator-request
Details | Review

Got confused by passing uri as a string here and not getting back what I expected, Johann asked me to file a bug to add a check here just so someone else doesnt make the same mistake in future

Priority: -- → P3

The idea here is that SitePermissions.getAllByURI, SitePermissions.get, SitePermissions.set and SitePermissions.remove take in an nsIURI as first parameter, but never really check that they actually got passed an nsIURI object (this can be done with uri instanceof Ci.nsIURI). Each of these functions should throw an error if the uri parameter is not an nsIURI.

I would like to take this up please :)

Go for it! Would you like to be unassigned from bug 1500464, then?

Assignee: nobody → phoenixgyaan
Status: NEW → ASSIGNED

Yes please, someone already has a patch for it :)

What are the STR for this.
Also, how do I confirm that the bug is fixed..???

Flags: needinfo?(jhofmann)

And do we need to throw a general error using try-catch block, or do we have a pre-defined string that will need to be shown.

Please take a look at comment 1, this is a code-only change, there are no user-facing STR for this. You can just throw an error using the JS throw keyword.

Thanks!

Flags: needinfo?(jhofmann)

functions now throw an error if the uri parameter is not an nsIURI.

Since our change might break some existing (wrong) usage, it would be great to do a try run for your patch. Try is basically Continuous Integration for Firefox, making sure that tests are passing for your patch.

Getting access is outlined here: https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_access_to_the_Try_Server

You will need to file a bug and need someone to vouch for you. I'd be happy to do that if you CC me on that bug.

After you've gotten access, to schedule a try run, you can use the handy ./mach try command. A possible syntax for this bug could be:

./mach try -b do -p win64,linux64,macosx64 -u mochitests -t none

(runs all mochitests on Windows, OSX and Linux)

Let me know if you need any help with that!

You will need to file a bug and need someone to vouch for you. I'd be happy to do that if you CC me on that bug.
Filed the bug here

Flags: needinfo?(jhofmann)

You will need to file a bug and need someone to vouch for you. I'd be happy to do that if you CC me on that bug.

Filed the bug here

Alright!

Flags: needinfo?(jhofmann)

Hi Johann,

I have got the access, is there anything that I need to do on my codebase now..???

or can I simply run the command ./mach try -b do -p win64,linux64,macosx64 -u mochitests -t none in my workspace..???

Flags: needinfo?(jhofmann)

Hey, you'll need to make sure you perform the steps outlined in https://wiki.mozilla.org/ReleaseEngineering/TryServer#Configuration and (most importantly) in https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/auth.html in order to get access on your local command line.

Then you can just run that command, yes :)

Flags: needinfo?(jhofmann)

Hi Johann,

I got it running :)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=942774ba2e27445bb68bb8fec86a57893cdb2e29

What is the next thing I should update you about..???

Flags: needinfo?(jhofmann)

Ran the tests again after the changes made in test files.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=388124b353a2075ec0496a480bf1a9a4436c5e4e

Oh, wow, amazing, you found some issues with your patch!

Going through the list of failures, here's what I see:

https://searchfox.org/mozilla-central/rev/7c20ad925005fbad7b8e08813115f1ec7fa1c248/browser/modules/PermissionUI.jsm#325

This one is PermissionsUI.jsm passing "null" as a URI because it only wants to get temporary permission from the browser object. That seems fine to me, and you should probably adjust our check in SitePermissions.get to allow a null URI if a browser was given (if ((!uri && !browser) || (uri && !(uri instanceof Ci.nsIURI)))).

https://searchfox.org/mozilla-central/rev/7c20ad925005fbad7b8e08813115f1ec7fa1c248/browser/components/preferences/in-content/tests/browser_permissions_dialog.js#58,69

These two are clearly wrong, they should pass URI instead.

https://searchfox.org/mozilla-central/rev/7c20ad925005fbad7b8e08813115f1ec7fa1c248/browser/base/content/test/webrtc/browser_devices_get_user_media_queue_request.js#85

This one probably means that we should apply the same null check for SitePermissions.remove and SitePermisions.set as well.

Can you make these changes in your patch?

Thank you!

Flags: needinfo?(jhofmann)

Hi,
The changes are done to the patch.

The tests' status can be seen at:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=25c8e523073fd3fc23f0b209b953741ff034497a

Flags: needinfo?(jhofmann)

That try looks good to me. You're all set to land your code! Please edit the bug (I've given you access to do so) and set the checkin-needed keyword. Then somebody will come around and check the patch in for you.

Thanks!

Flags: needinfo?(jhofmann)
Attachment #9051289 - Flags: checkin+

Comment on attachment 9051289 [details]
Bug 1521919 - SitePermissions.get should check for nsIURI. r=johannh

Sorry, a different field, I'll do it :)

Attachment #9051289 - Flags: checkin+

Sorry, I never did this before, so didn't knew which one it was.

Thank you :)

Pushed by rmaries@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/01cdf8342a49
SitePermissions.get should check for nsIURI. r=johannh

Keywords: checkin-needed

Johann, how do I proceed with this..???

Flags: needinfo?(phoenixgyaan) → needinfo?(jhofmann)

It seems like your test is failing on CI. Looking at it again it seems like the regex you're providing won't match the error, I think. Sorry for not catching that earlier.

Did you run the test locally at all? Unfortunately your try run didn't fail either, because I didn't specify xpcshell in the syntax, so after fixing the test locally you should do another try like this:

./mach try -b do -p win64,linux64,macosx64 -u xpcshell -t none
Flags: needinfo?(jhofmann)

On running tests locally using mach xpcshell-test I got the following output at the end

xpcshell
~~~~~~~~
Ran 3672 checks (180 subtests, 3492 tests)
Expected results: 3439
Skipped: 203 tests
Unexpected results: 30
  test: 18 (18 fail)
  subtest: 12 (12 fail)

Unexpected Results
------------------
dom/push/test/xpcshell/test_register_success_http2.js
  FAIL dom/push/test/xpcshell/test_register_success_http2.js - xpcshell return code: 0
dom/push/test/xpcshell/test_register_error_http2.js
  FAIL dom/push/test/xpcshell/test_register_error_http2.js - xpcshell return code: 0
dom/push/test/xpcshell/test_unregister_success_http2.js
  FAIL dom/push/test/xpcshell/test_unregister_success_http2.js - xpcshell return code: 0
dom/push/test/xpcshell/test_notification_http2.js
  FAIL dom/push/test/xpcshell/test_notification_http2.js - xpcshell return code: 0
dom/push/test/xpcshell/test_registration_success_http2.js
  FAIL dom/push/test/xpcshell/test_registration_success_http2.js - xpcshell return code: 0
netwerk/test/unit/test_immutable.js
  FAIL netwerk/test/unit/test_immutable.js - xpcshell return code: 0
  FAIL run_test - [run_test : 13] "" != ""
i:/Mozilla/mozilla-central/objdir-frontend/_tests/xpcshell/netwerk/test/unit/test_immutable.js:run_test:13
i:\Mozilla\mozilla-central\testing\xpcshell\head.js:_execute_test:520
-e:null:1
netwerk/test/unit/test_http2.js
  FAIL netwerk/test/unit/test_http2.js - xpcshell return code: 0
  FAIL testOnStopRequest - [testOnStopRequest : 79] false == true
i:/Mozilla/mozilla-central/objdir-frontend/_tests/xpcshell/netwerk/test/unit/test_http2.js:testOnStopRequest:79
i:\Mozilla\mozilla-central\testing\xpcshell\head.js:_do_main:224
i:\Mozilla\mozilla-central\testing\xpcshell\head.js:_execute_test:526
-e:null:1
netwerk/test/unit/test_altsvc.js
  FAIL netwerk/test/unit/test_altsvc.js - xpcshell return code: 0
  FAIL run_test - [run_test : 28] "" != ""
i:/Mozilla/mozilla-central/objdir-frontend/_tests/xpcshell/netwerk/test/unit/test_altsvc.js:run_test:28
i:\Mozilla\mozilla-central\testing\xpcshell\head.js:_execute_test:520
-e:null:1
netwerk/test/unit/test_header_Server_Timing.js
  FAIL netwerk/test/unit/test_header_Server_Timing.js - xpcshell return code: 0
browser/modules/test/unit/test_SitePermissions.js
  FAIL browser/modules/test/unit/test_SitePermissions.js - xpcshell return code: 0
  FAIL testNsIURI - [testNsIURI : 15] Missing expected exception. Should throw if arguments is not of type nsIURI.
i:/Mozilla/mozilla-central/objdir-frontend/_tests/xpcshell/browser/modules/test/unit/test_SitePermissions.js:testNsIURI:15
i:\Mozilla\mozilla-central\testing\xpcshell\head.js:run_next_test/_run_next_test/<:1434
i:\Mozilla\mozilla-central\testing\xpcshell\head.js:_run_next_test:1434
i:\Mozilla\mozilla-central\testing\xpcshell\head.js:run:685
i:\Mozilla\mozilla-central\testing\xpcshell\head.js:_do_main:224
i:\Mozilla\mozilla-central\testing\xpcshell\head.js:_execute_test:526
-e:null:1
netwerk/test/unit/test_anonymous-coalescing.js
  FAIL netwerk/test/unit/test_anonymous-coalescing.js - xpcshell return code: 0
  FAIL run_test - [run_test : 20] "" != ""
i:/Mozilla/mozilla-central/objdir-frontend/_tests/xpcshell/netwerk/test/unit/test_anonymous-coalescing.js:run_test:20
i:\Mozilla\mozilla-central\testing\xpcshell\head.js:_execute_test:520
-e:null:1
netwerk/test/unit/test_origin.js
  FAIL netwerk/test/unit/test_origin.js - xpcshell return code: 0
  FAIL run_test - [run_test : 14] "" != ""
i:/Mozilla/mozilla-central/objdir-frontend/_tests/xpcshell/netwerk/test/unit/test_origin.js:run_test:14
i:\Mozilla\mozilla-central\testing\xpcshell\head.js:_execute_test:520
-e:null:1
netwerk/test/unit/test_trr.js
  FAIL netwerk/test/unit/test_trr.js - xpcshell return code: 0
  FAIL run_test - [run_test : 19] "" != ""
i:/Mozilla/mozilla-central/objdir-frontend/_tests/xpcshell/netwerk/test/unit/test_trr.js:run_test:19
i:\Mozilla\mozilla-central\testing\xpcshell\head.js:_execute_test:520
-e:null:1
netwerk/test/unit/test_esni_dns_fetch.js
  FAIL netwerk/test/unit/test_esni_dns_fetch.js - xpcshell return code: 0
  FAIL run_test - [run_test : 17] "" != ""
i:/Mozilla/mozilla-central/objdir-frontend/_tests/xpcshell/netwerk/test/unit/test_esni_dns_fetch.js:run_test:17
i:\Mozilla\mozilla-central\testing\xpcshell\head.js:_execute_test:520
-e:null:1
security/manager/ssl/tests/unit/test_nonascii_path.js
  FAIL security/manager/ssl/tests/unit/test_nonascii_path.js - xpcshell return code: 0
  FAIL undefined assertion name - the profile short path should contain a non-ASCII character - false == true
i:/Mozilla/mozilla-central/objdir-frontend/_tests/xpcshell/security/manager/ssl/tests/unit/test_nonascii_path.js:null:24
i:\Mozilla\mozilla-central\testing\xpcshell\head.js:load_file:634
i:\Mozilla\mozilla-central\testing\xpcshell\head.js:_load_files:646
i:\Mozilla\mozilla-central\testing\xpcshell\head.js:_execute_test:500
-e:null:1
toolkit/components/crashes/tests/xpcshell/test_crash_manager.js
  FAIL toolkit/components/crashes/tests/xpcshell/test_crash_manager.js - xpcshell return code: 0
  FAIL test_child_process_crash_ping - [test_child_process_crash_ping : 532] 1552633200000 == 1552635000000
i:/Mozilla/mozilla-central/objdir-frontend/_tests/xpcshell/toolkit/components/crashes/tests/xpcshell/test_crash_manager.js:test_child_process_crash_ping:532
toolkit/components/search/tests/xpcshell/test_geodefaults.js
  FAIL toolkit/components/search/tests/xpcshell/test_geodefaults.js - xpcshell return code: 0
  FAIL should_get_geo_defaults_only_once - [should_get_geo_defaults_only_once : 49] "undefined" == "number"
i:/Mozilla/mozilla-central/objdir-frontend/_tests/xpcshell/toolkit/components/search/tests/xpcshell/test_geodefaults.js:cont:49
toolkit/mozapps/update/tests/unit_service_updater/checkUpdaterSigSvc.js
  FAIL toolkit/mozapps/update/tests/unit_service_updater/checkUpdaterSigSvc.js - xpcshell return code: 0
  FAIL run_test - [run_test : 37] the maintenance service exit value should be 0 - 1 == 0
i:/Mozilla/mozilla-central/objdir-frontend/_tests/xpcshell/toolkit/mozapps/update/tests/unit_service_updater/checkUpdaterSigSvc.js:run_test:37
i:\Mozilla\mozilla-central\testing\xpcshell\head.js:_execute_test:520
-e:null:1
Tests were run in parallel. Try running with --sequential to make sure the failures were not caused by this.

The only line where my changed file failed was at

browser/modules/test/unit/test_SitePermissions.js
  FAIL browser/modules/test/unit/test_SitePermissions.js - xpcshell return code: 0
  FAIL testNsIURI - [testNsIURI : 15] Missing expected exception. Should throw if arguments is not of type nsIURI.

So, is this regex causing the failure of tests ->/Not_nsIURI_Error/

Flags: needinfo?(jhofmann)

Hi Johann I am stuck here.

I have changed the code to

.
.
.
add_task(async function testNsIURI() {
let uri = "http://foo.com/bar/baz";
  const expectedError = /uri parameter should be an nsIURI/;

  Assert.throws(() => SitePermissions.getAllByURI(uri),
                expectedError,
                "Should throw if arguments is not of type nsIURI.");
.
.
.

But still the test result comes out as

.
.
.
xpcshell
~~~~~~~~
Ran 2 checks (1 subtests, 1 tests)
Expected results: 0
Unexpected results: 2
  test: 1 (1 fail)
  subtest: 1 (1 fail)

Unexpected Results
------------------
browser/modules/test/unit/test_SitePermissions.js
  FAIL testNsIURI - [testNsIURI : 16] Missing expected exception. Should throw if arguments is not of type nsIURI.
i:/Mozilla/mozilla-central/objdir-frontend/_tests/xpcshell/browser/modules/test/unit/test_SitePermissions.js:testNsIURI:16
.
.
.

This works fine for me, can you please upload the updated patch so that I have more context to that failure?

Flags: needinfo?(jhofmann) → needinfo?(phoenixgyaan)

I have updated the patch in Phabricator.
Please have a look

Flags: needinfo?(phoenixgyaan) → needinfo?(jhofmann)

Hey, I'm really sorry for the delay, this is working fine for me and I can't see why it wouldn't. I'm going to land this for you :)

Thanks!

Flags: needinfo?(jhofmann)
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d66081bb98b8
SitePermissions.get should check for nsIURI. r=johannh

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

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
See Also: → 1583790
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: