1497301 - Remove some dead code in Location

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Boris Zbarsky [:bzbarsky]]

We have some Location methods that call SetHrefWithContext if there is a current JSContext, else do some fallback stuff.  The only callers are bindings, so there is always a JSContext.  Going to make that explicit and remove the dead code.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Remove the JSContext arg of Location::GetSourceBaseURL

It's unused.  Also, this function never fails, so there's no reason to have the
nsresult return type.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Remove some dead code in Location methods

The JSContext arg of SetHrefWithContext is unused, and the function should have
a different name.

All the current callers of SetHrefWithContext are always called from bindings,
so we can't hit the "no jscontext" cases.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Simplify Location::CheckURL

The main change is to just use the principal bindings pass us to do our
CheckLoadURI check.  If we do that, we don't have to care about the current
JSContext.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3 diff -w

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 9015360 [details] [diff] [review]
Part 3 diff -w

Review of attachment 9015360 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM modulo that.

::: dom/base/Location.cpp
@@ +78,5 @@
>      NS_ENSURE_STATE(ssm);
>  
>      // Check to see if URI is allowed.
> +  nsresult rv = ssm->CheckLoadURIWithPrincipal(&aSubjectPrincipal, aURI,
> +                                               nsIScriptSecurityManager::STANDARD);

So, this will stop throwing the exception that CheckLoadURIFromScript throws. Is that ok?

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 9015358 [details] [diff] [review]
part 3.  Simplify Location::CheckURL

I reviewed the diff -w.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Is that ok?

I think so.  We'll still throw (NS_ERROR_FAILURE).  Long-term, I suspect we will want to no-op instead of throwing anyway, per spec...

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9340d1867b1f
part 1.  Remove the JSContext arg of Location::GetSourceBaseURL.  r=farre
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9e04519cd76
part 2.   Remove some dead code in Location methods.  r=farre
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d0df5f74170
part 3.  Simplify Location::CheckURL.  r=bholley

Comment 9

[Raul Gurzau (:RaulG)]

Backed out changeset 5d0df5f74170 (bug 1497301) for failing at caps/tests/ and js/xpconnect/tests on a CLOSED TREE

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/9268ec81fb01ae1fd1162fc6866ef8f74cd3ea09

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed,busted,exception&revision=5d0df5f741700ae8e74d894ae95ae590b239e0ec&selectedJob=205093033

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=205093033&repo=mozilla-inbound&lineNumber=4822

Log snippet: 
[task 2018-10-12T15:39:06.507Z] 15:39:06     INFO - TEST-START | caps/tests/mochitest/test_bug246699.html
[task 2018-10-12T15:39:06.630Z] 15:39:06     INFO - GECKO(2421) | ++DOMWINDOW == 41 (0xd877d800) [pid = 2421] [serial = 41] [outer = 0xd830ca60]
[task 2018-10-12T15:39:06.686Z] 15:39:06     INFO - GECKO(2421) | ++DOCSHELL 0xd85c5000 == 13 [pid = 2421] [id = {ecec7b9d-5aa9-4b2c-803f-9e6e439fd032}]
[task 2018-10-12T15:39:06.687Z] 15:39:06     INFO - GECKO(2421) | ++DOMWINDOW == 42 (0xd7c423a0) [pid = 2421] [serial = 42] [outer = (nil)]
[task 2018-10-12T15:39:06.703Z] 15:39:06     INFO - GECKO(2421) | ++DOMWINDOW == 43 (0xdda65800) [pid = 2421] [serial = 43] [outer = 0xd7c423a0]
[task 2018-10-12T15:39:06.719Z] 15:39:06     INFO - GECKO(2421) | [2421, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805303F4: file /builds/worker/workspace/build/src/dom/base/Location.cpp, line 83
[task 2018-10-12T15:39:06.729Z] 15:39:06     INFO - TEST-INFO | started process screentopng
[task 2018-10-12T15:39:07.199Z] 15:39:07     INFO - TEST-INFO | screentopng: exit 0
[task 2018-10-12T15:39:07.199Z] 15:39:07     INFO - TEST-UNEXPECTED-FAIL | caps/tests/mochitest/test_bug246699.html | should get stack for content-loading-chrome rejection - got "unexpected: [Exception... \"Failure\"  nsresult: \"0x80004005 (NS_ERROR_FAILURE)\"  location: \"JS frame :: http://mochi.test:8888/tests/caps/tests/mochitest/test_bug246699.html :: tryChromeLoad :: line 45\"  data: no]", expected "denied-stack"
[task 2018-10-12T15:39:07.199Z] 15:39:07     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:312:5
[task 2018-10-12T15:39:07.201Z] 15:39:07     INFO -     @caps/tests/mochitest/test_bug246699.html:53:1
[task 2018-10-12T15:39:07.201Z] 15:39:07     INFO - TEST-PASS | caps/tests/mochitest/test_bug246699.html | should get stack for SpecialPowers.Components.classes rejection

Comment 10

[Boris Zbarsky [:bzbarsky]]

We had tests relying on the old exception behavior.  I will more or less restore it for now.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Change a bunch of Location function signatures to take an ErrorResult

This way we can propagate errors out usefully.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff from the old part 3 to the new part 4, which fixes the failing tests

For now, I'm just keeping the structure of the exception, except it becomes a TypeError instead of an Error.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Simplify Location::CheckURL

The main change is to just use the principal bindings pass us to do our
CheckLoadURI check.  If we do that, we don't have to care about the current
JSContext.
* * *
Bug 1497301 patch

Comment 14

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/9340d1867b1f
https://hg.mozilla.org/mozilla-central/rev/c9e04519cd76

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bd7470927d2
part 3.  Change a bunch of Location function signatures to take an ErrorResult.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/45ba0943d666
part 4.  Simplify Location::CheckURL.  r=bholley

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 2 changesets (Bug 1497301) for /url/failure.html failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=45ba0943d6661edcc71490910e8cdeaa8326ade2

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/4c77e2acad03dea20d2f1c4a63935e43d146e758

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=205617571&repo=mozilla-inbound&lineNumber=23889

[task 2018-10-15T21:04:51.597Z] 21:04:51     INFO - TEST-FAIL | /url/failure.html | URL's href: file://example:1/ should throw - assert_throws: function "() => url.href = test.input" did not throw
[task 2018-10-15T21:04:51.598Z] 21:04:51     INFO - runTests/<@http://web-platform.test:8000/url/failure.html:27:7
[task 2018-10-15T21:04:51.598Z] 21:04:51     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1561:20
[task 2018-10-15T21:04:51.599Z] 21:04:51     INFO - test@http://web-platform.test:8000/resources/testharness.js:544:21
[task 2018-10-15T21:04:51.599Z] 21:04:51     INFO - runTests@http://web-platform.test:8000/url/failure.html:25:5
[task 2018-10-15T21:04:51.600Z] 21:04:51     INFO - TEST-FAIL | /url/failure.html | XHR: file://example:1/ should throw - assert_throws: function "() => client.open("GET", test.input)" did not throw
[task 2018-10-15T21:04:51.600Z] 21:04:51     INFO - runTests/<@http://web-platform.test:8000/url/failure.html:32:7
[task 2018-10-15T21:04:51.601Z] 21:04:51     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1561:20
[task 2018-10-15T21:04:51.601Z] 21:04:51     INFO - test@http://web-platform.test:8000/resources/testharness.js:544:21
[task 2018-10-15T21:04:51.601Z] 21:04:51     INFO - runTests@http://web-platform.test:8000/url/failure.html:30:5
[task 2018-10-15T21:04:51.602Z] 21:04:51     INFO - TEST-PASS | /url/failure.html | sendBeacon(): file://example:1/ should throw 
[task 2018-10-15T21:04:51.603Z] 21:04:51     INFO - TEST-UNEXPECTED-PASS | /url/failure.html | Location's href: file://example:1/ should throw - expected FAIL
[task 2018-10-15T21:04:51.603Z] 21:04:51     INFO - TEST-INFO | expected FAIL

Comment 17

[Boris Zbarsky [:bzbarsky]]

Argh, unexpected passes.... I missed those in my try run somehow.  Ah, well.  Just need to adjust the test expectations.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08174b4a8022
part 3.  Change a bunch of Location function signatures to take an ErrorResult.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/90b73f4b4635
part 4.  Simplify Location::CheckURL.  r=bholley

Comment 19

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/08174b4a8022
https://hg.mozilla.org/mozilla-central/rev/90b73f4b4635