Closed Bug 1018682 Opened 8 years ago Closed 8 years ago

"ASSERTION: Non-chrome URI?" when a messed-with URL object as a base URL

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox33 --- verified

People

(Reporter: jruderman, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 4 obsolete files)

Attached file testcase
###!!! ASSERTION: Non-chrome URI?: 'isChrome', file chrome/src/nsChromeRegistry.cpp, line 155
Attached file stack
SetProtocol should probably be creating a new nsIURI or something instead of blindly dong SetScheme.  SetScheme is a major footgun...

URL seems to be the only code that exposes a URI that's been messed with this way to script.  It needs to stop doing that.  For example, it could SetScheme and then GetHref and NS_NewURI that string or something (that's what setting .protocol on <a> does, say).
Component: Networking → DOM
Flags: needinfo?(amarchesini)
Per the specification it should set the scheme (but not change the relative flag of the URL). But then in the specification there's not a per-scheme handler as there is in Gecko. It's not entirely clear to me what the correct solution is here.
Attached patch urlchrome.patch (obsolete) — Splinter Review
This patch "fixes" this issue but it doesn't allow the changing of the scheme from data: to chrome:.
Attachment #8432449 - Flags: feedback?(bzbarsky)
Flags: needinfo?(amarchesini)
Comment on attachment 8432449 [details] [diff] [review]
urlchrome.patch

Forgot to qref?
Attachment #8432449 - Flags: feedback?(bzbarsky) → feedback-
Attached patch urlchrome.patch (obsolete) — Splinter Review
Attachment #8432449 - Attachment is obsolete: true
Attachment #8432479 - Flags: feedback?(bzbarsky)
Comment on attachment 8432479 [details] [diff] [review]
urlchrome.patch

Why do we need all those changes to non-URL code?  And why are we doing hand-parsing of URLs like this anyway?

What's wrong with just making URL do what <a> does right now: setting protocol does the SetScheme, but then round-trips through a string?
Attachment #8432479 - Flags: feedback?(bzbarsky) → feedback-
Attached patch urlchrome.patch (obsolete) — Splinter Review
Attachment #8432479 - Attachment is obsolete: true
Attachment #8432517 - Flags: review?(bzbarsky)
Attached patch urlchrome.patch (obsolete) — Splinter Review
This patch throws if the protocol cannot be set but we can do no-op as many other methods of URL API. bz, what do you suggest?
Attachment #8432517 - Attachment is obsolete: true
Attachment #8432517 - Flags: review?(bzbarsky)
Attachment #8434205 - Flags: review?(bzbarsky)
Comment on attachment 8434205 [details] [diff] [review]
urlchrome.patch

I think you should talk to Anne about what behavior the spec has here.  The spec does do scheme-specific parsing, so it should at least think about what happens when protocol is switched to/from the list of things that are parsed specially.
I have no strong opinions on what is best here. Currently you can just change scheme and then things will fail once you do a fetch or some such.
Comment on attachment 8434205 [details] [diff] [review]
urlchrome.patch

I think we should no-op on failure, as elsewhere.
Attachment #8434205 - Flags: review?(bzbarsky) → review-
Attached patch urlchrome.patchSplinter Review
Attachment #8434205 - Attachment is obsolete: true
Attachment #8435564 - Flags: review?(bzbarsky)
Blocks: 1016277
Comment on attachment 8435564 [details] [diff] [review]
urlchrome.patch

>+  // if we change it to not reserialize we need to do something smarter in

s/it/this code/

>+  nsresult rv = clone->SetScheme(NS_ConvertUTF16toUTF8(Substring(start, iter)));

Please null-check clone first.

Also, add a comment here explaining why we do the serialize/reparse, please.

>+  nsCOMPtr<nsIIOService> ioService(do_GetService(NS_IOSERVICE_CONTRACTID, &rv));

NS_NewURI is your friend.

r=me with those fixed.
Attachment #8435564 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/ec32b9724221
Assignee: nobody → amarchesini
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Reproduced in Nightly 2014-05-30-mozilla-central-debug.
Verified fixed FF 33.0a1 2014-06-13-mozilla-central-debug Win 7 x64.
Status: RESOLVED → VERIFIED
No longer blocks: 1016277
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.