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

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Non-chrome URI?: 'isChrome', file chrome/src/nsChromeRegistry.cpp, line 155

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Comment 3

[Anne (:annevk)]

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.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: urlchrome.patch

This patch "fixes" this issue but it doesn't allow the changing of the scheme from data: to chrome:.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8432449 [details] [diff] [review]
urlchrome.patch

Forgot to qref?

Comment 6

[Andrea Marchesini [:baku]]

Attachment: urlchrome.patch

Comment 7

[Boris Zbarsky [: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?

Comment 8

[Andrea Marchesini [:baku]]

Attachment: urlchrome.patch

Comment 9

[Andrea Marchesini [:baku]]

Attachment: urlchrome.patch

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?

Comment 10

[Boris Zbarsky [: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.

Comment 11

[Anne (:annevk)]

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 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8434205 [details] [diff] [review]
urlchrome.patch

I think we should no-op on failure, as elsewhere.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: urlchrome.patch

Comment 14

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec32b9724221

https://tbpl.mozilla.org/?tree=Try&rev=3236ebc37b71

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Paul Silaghi, QA [:pauly]]

Reproduced in Nightly 2014-05-30-mozilla-central-debug.
Verified fixed FF 33.0a1 2014-06-13-mozilla-central-debug Win 7 x64.