Closed Bug 1497301 Opened Last year Closed Last year

Remove some dead code in Location

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 1 obsolete file)

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.
It's unused.  Also, this function never fails, so there's no reason to have the
nsresult return type.
Attachment #9015354 - Flags: review?(afarre)
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.
Attachment #9015355 - Flags: review?(afarre)
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.
Attachment #9015358 - Flags: review?(bobbyholley)
Priority: -- → P2
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?
Attachment #9015360 - Flags: review+
Comment on attachment 9015358 [details] [diff] [review]
part 3.  Simplify Location::CheckURL

I reviewed the diff -w.
Attachment #9015358 - Flags: review?(bobbyholley)
> 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...
Attachment #9015354 - Flags: review?(afarre) → review+
Attachment #9015355 - Flags: review?(afarre) → review+
Pushed by bzbarsky@mozilla.com:
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
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
Flags: needinfo?(bzbarsky)
We had tests relying on the old exception behavior.  I will more or less restore it for now.
This way we can propagate errors out usefully.
Attachment #9016748 - Flags: review?(bobbyholley)
For now, I'm just keeping the structure of the exception, except it becomes a TypeError instead of an Error.
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
Attachment #9016751 - Flags: review?(bobbyholley)
Attachment #9015358 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/9340d1867b1f
https://hg.mozilla.org/mozilla-central/rev/c9e04519cd76
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #9016748 - Flags: review?(bobbyholley) → review+
Attachment #9016751 - Flags: review?(bobbyholley) → review+
Pushed by bzbarsky@mozilla.com:
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
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
Flags: needinfo?(bzbarsky)
Argh, unexpected passes.... I missed those in my try run somehow.  Ah, well.  Just need to adjust the test expectations.
Flags: needinfo?(bzbarsky)
Pushed by bzbarsky@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/08174b4a8022
https://hg.mozilla.org/mozilla-central/rev/90b73f4b4635
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Flags: qe-verify-
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.