Closed Bug 1131406 Opened 5 years ago Closed 5 years ago

Serializing most nsSimpleURIs loses information

Categories

(Core :: DOM: Content Processes, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
e10s + ---
firefox39 --- fixed

People

(Reporter: mrbkap, Assigned: mrbkap)

Details

Attachments

(3 files, 5 obsolete files)

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.
Right now, the affected URI classes are: nsJSURI, nsNestedSimpleURI, and nsHostObjectURI.
Oops, that's nsSimpleNestedURI.
Attached patch Fix nsJSURI (obsolete) — Splinter Review
Attached patch Fix nsSimpleNestedURI (obsolete) — Splinter Review
Attachment #8571655 - Attachment is obsolete: true
Attached patch Fix nsHostObjectURI (obsolete) — Splinter Review
Attachment #8571656 - Attachment is obsolete: true
Attachment #8571654 - Flags: review?(wmccloskey)
Attachment #8572264 - Flags: review?(wmccloskey)
Attachment #8572265 - Flags: review?(wmccloskey)
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?
Attachment #8571654 - Flags: review?(wmccloskey) → review+
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.
Attachment #8572264 - Flags: review?(wmccloskey) → review+
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 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.
Attachment #8572265 - Flags: review?(wmccloskey) → review+
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.
Keywords: checkin-needed
Attached patch Fix nsJSURISplinter Review
Attachment #8571654 - Attachment is obsolete: true
Attachment #8572264 - Attachment is obsolete: true
Attachment #8572265 - Attachment is obsolete: true
Attachment #8574255 - Flags: review+
You need to log in before you can comment on or make changes to this bug.