1193552 - Remove baseURI from LoadInfo

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb}]

I just realized that we do not serialize the baseURI for e10s, we should get that fixed!

Comment 1

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1193552_serialize_baseuri.patch

Hey Bill, apparently the baseURI was never serialized. I would like to keep things consistent. Can you review that patch for me?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Noooo :(

Can't we kill the baseURI property of loadInfo. It was never well thought out and was added to fix something that IMHO wasn't really a problem.

Comment 3

[Christoph Kerschbaumer [:ckerschb}]

Comment on attachment 8646645 [details] [diff] [review]
bug_1193552_serialize_baseuri.patch

Either way, I thought we want things in sync between regular and e10s mode. If we decide to keep the baseURI, then we should land that patch as well, if we decide to move it back to where it was, then all good.

Comment 4

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1193552_remove_baseuri_from_loadinfo.patch

(In reply to Jonas Sicking (:sicking) from comment #2)
> Noooo :(
> 
> Can't we kill the baseURI property of loadInfo. It was never well thought
> out and was added to fix something that IMHO wasn't really a problem.

Let's give it a try :-)


James, in comment [1] you mentioned you don't have any objections to reverting the change. It took a while but I think we would like to move the baseURI back where it was. (Unless Jonas as any specific ideas of what we should do).

Unfortunately a simple back out of the patch didn't work anymore, hence I manually converted all the changes. Are there any automated testcases to make sure it works? If not, how can I manually test to make sure everything is fine?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=971432#c12

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8647108 [details] [diff] [review]
bug_1193552_remove_baseuri_from_loadinfo.patch

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

Thanks!

::: docshell/base/nsDocShell.cpp
@@ +10526,5 @@
>                                        triggeringPrincipal.get(),
>                       triggeringPrincipal,
>                       requestingNode,
>                       securityFlags,
> +                     aContentPolicyType);

Any chance we can move this into the NewSrcdocChannel function? I'd rather not have "standalone" code which creates LoadInfo's and calls SetLoadInfo on existing channels. Especially since this might be copied to other parts of the code.

I.e. make NewSrcDocChannel take a bunch of additional arguments, and then have NewSrcDocChannel create the LoadInfo and set it on the channel before returning it.

Comment 6

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1193552_remove_baseuri_from_loadinfo.patch

(In reply to Jonas Sicking (:sicking) from comment #5)
> Any chance we can move this into the NewSrcdocChannel function? I'd rather
> not have "standalone" code which creates LoadInfo's and calls SetLoadInfo on
> existing channels. Especially since this might be copied to other parts of
> the code.

Good call - I totally agree. As it turns out, the function nsViewSourceChannel::InitSrcdoc() internally creates a new channel by calling NS_NewInputStreamChannel. So we can just pass the arguments along to netuil.h, which is great in the end! Nice!

[Carrying over r+ from Jonas]

Comment 7

[James Kitchener (:jkitch)]

https://bugzilla.mozilla.org/attachment.cgi?id=8365919

I never worked out an automated test for it.

Because of that, view source of srcdoc iframe broke and nobody noticed.  Currently running mozregression to find out when.


The linked testcase can still be used for view selection source.  Just highlight the link marked "bad".

Comment 8

[James Kitchener (:jkitch)]

Actually there is a test for view selection source.

toolkit/components/viewsource/test/browser/browser_bug464222.js

Comment 9

[Christoph Kerschbaumer [:ckerschb}]

James, trying to make sure we are doing the right thing here. First, the automated test [1] works. Now I am running manual tests from [2], where I get 3 iframes and perform right-click->This_Frame->View_Frame_Source on each of them.

> <iframe srcdoc='<a href="about:mozilla">good</a>'></iframe>
> <iframe srcdoc='<a href="https://bugzilla.mozilla.org">good</a>'></iframe>
> <iframe srcdoc='<a href="viewframe.html">bad</a>'></iframe>

I get the same result on all 3 of them:
> The address isn't valid

Reason is, that we are about to create a channel for 'view-source:about:srcdoc' which is *not* permitted by the aboutProtocolhandler [3].
Even without my patch applied I get the same result at the moment. I suppose view-source should work for all 3 of them, right?

So, we really have to find what caused that regression!

[1] toolkit/components/viewsource/test/browser/browser_bug464222.js
[2] https://bugzilla.mozilla.org/attachment.cgi?id=8365919
[3] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsAboutProtocolHandler.cpp#141

Comment 10

[James Kitchener (:jkitch)]

I'm working on finding the regressing patch, however it seems to have been present since November last year.

Comment 11

[James Kitchener (:jkitch)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c6a3db6682e0&tochange=060f
9a5bdcaf

Bug 1025146 seems to be the most likely candidate.

(The November problem was e10s-related).

Comment 12

[Christoph Kerschbaumer [:ckerschb}]

(In reply to James Kitchener (:jkitch) from comment #11)
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=c6a3db6682e0&tochange=060f
> 9a5bdcaf
> 
> Bug 1025146 seems to be the most likely candidate.
> 
> (The November problem was e10s-related).

Thanks James, I filed bug 1194764 and marked it blocking this one. Let's see if we can get that resolved quickly.

Comment 13

[Christoph Kerschbaumer [:ckerschb}]

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

Comment 14

[Christoph Kerschbaumer [:ckerschb}]

Just chatted with Jryans on IRC; we both think that bug 1194764 is not really blocking this one, so I will go ahead and land this patch to avoid bitrot of the patch.

Comment 15

[Christoph Kerschbaumer [:ckerschb}]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/495b6ed690b2f0d5b01668cdc0e7cde9b66b5803
changeset:  495b6ed690b2f0d5b01668cdc0e7cde9b66b5803
user:       Christoph Kerschbaumer <[e-mail]>
date:       Wed Aug 19 10:43:30 2015 -0700
description:
Bug 1193552 - Remove baseURI from LoadInfo (r=sicking,jkitch)

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/495b6ed690b2