1131406 - Serializing most nsSimpleURIs loses information

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

While working on bug 1117337, I found that the way that we serialize nsSimpleURIs loses information and creates the wrong concrete type on the receiving end of the serialized URI. For example, if we serialize and then deserialize an nsJSURI, we create an nsSimpleURI and lose the base URI.

We should fix this and remove the footgun.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Right now, the affected URI classes are: nsJSURI, nsNestedSimpleURI, and nsHostObjectURI.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Oops, that's nsSimpleNestedURI.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix nsJSURI

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix nsSimpleNestedURI.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix nsHostObjectURI.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix nsSimpleNestedURI

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix nsHostObjectURI

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8571654 [details] [diff] [review]
Fix nsJSURI

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

::: dom/jsurl/nsJSProtocolHandler.cpp
@@ +1271,5 @@
>        *aInstancePtr = nullptr;
>        return NS_NOINTERFACE;
>    }
>    else
> +  NS_INTERFACE_MAP_ENTRY(nsIIPCSerializableURI)

I wouldn't think you'd need this. Is there any reason to have it here?

@@ +1327,5 @@
> +
> +    jsParams.simpleParams() = simpleParams;
> +    if (mBaseURI) {
> +        URIParams baseParams;
> +        SerializeURI(mBaseURI, baseParams);

Can you just pass jsParams.baseURI() as the second param here and drop the temporary?

@@ +1351,5 @@
> +    nsSimpleURI::Deserialize(jsParams.simpleParams());
> +
> +    if (jsParams.baseURI().type() != OptionalURIParams::Tvoid_t) {
> +        mBaseURI = DeserializeURI(jsParams.baseURI().get_URIParams());
> +    }

Maybe set mBaseURI to null explicitly in the other case?

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8572264 [details] [diff] [review]
Fix nsSimpleNestedURI

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

::: netwerk/protocol/viewsource/moz.build
@@ +16,5 @@
>  ]
>  
>  FAIL_ON_WARNINGS = True
>  
> +include('/ipc/chromium/chromium-config.mozbuild')

Can you remind me what this is for? And maybe comment why it's needed here.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8572264 [details] [diff] [review]
Fix nsSimpleNestedURI

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

::: netwerk/base/nsSimpleNestedURI.h
@@ +19,5 @@
>  #include "nsCOMPtr.h"
>  #include "nsSimpleURI.h"
>  #include "nsINestedURI.h"
>  
> +#include "mozilla/ipc/URIUtils.h"

Do you need URIUtils here? Maybe if you took that out you wouldn't need the weird chromium mozbuild business.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8572265 [details] [diff] [review]
Fix nsHostObjectURI

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

::: dom/base/nsHostObjectURI.cpp
@@ +22,5 @@
>  NS_IMPL_RELEASE_INHERITED(nsHostObjectURI, nsSimpleURI)
>  
>  NS_INTERFACE_MAP_BEGIN(nsHostObjectURI)
>    NS_INTERFACE_MAP_ENTRY(nsIURIWithPrincipal)
> +  NS_INTERFACE_MAP_ENTRY(nsIIPCSerializableURI)

Again, I wonder if you need this. It seems like the INHERITING should take care of it.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=777c020886ff

I ended up not needing to change viewsource's moz.build. I included the right file in nsNestedSimpleURI.h instead.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix nsJSURI

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix nsSimpleNestedURI

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix nsHostObjectURI

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c07129c1be66
https://hg.mozilla.org/integration/mozilla-inbound/rev/90c42f366274
https://hg.mozilla.org/integration/mozilla-inbound/rev/f72575138bef

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c07129c1be66
https://hg.mozilla.org/mozilla-central/rev/90c42f366274
https://hg.mozilla.org/mozilla-central/rev/f72575138bef