Closed Bug 1193552 Opened 4 years ago Closed 4 years ago

Remove baseURI from LoadInfo

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

Attachments

(1 file, 2 obsolete files)

I just realized that we do not serialize the baseURI for e10s, we should get that fixed!
Assignee: nobody → mozilla
Depends on: 971432
Blocks: 971432
No longer depends on: 971432
Hey Bill, apparently the baseURI was never serialized. I would like to keep things consistent. Can you review that patch for me?
Attachment #8646645 - Flags: review?(wmccloskey)
Attachment #8646645 - Flags: review?(wmccloskey) → review+
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.
Summary: baseURI within LoadInfo not serialized → Remove baseURI from LoadInfo
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.
Attachment #8646645 - Attachment is obsolete: true
(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
Flags: needinfo?(jkitch.bug)
Attachment #8647108 - Flags: review?(jonas)
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.
Attachment #8647108 - Flags: review?(jonas) → review+
(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]
Attachment #8647108 - Attachment is obsolete: true
Attachment #8647195 - Flags: review+
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".
Flags: needinfo?(jkitch.bug)
Actually there is a test for view selection source.

toolkit/components/viewsource/test/browser/browser_bug464222.js
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
Flags: needinfo?(jkitch.bug)
I'm working on finding the regressing patch, however it seems to have been present since November last year.
Flags: needinfo?(jkitch.bug)
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).
Depends on: 1194764
(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.
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.
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/495b6ed690b2f0d5b01668cdc0e7cde9b66b5803
changeset:  495b6ed690b2f0d5b01668cdc0e7cde9b66b5803
user:       Christoph Kerschbaumer <mozilla@christophkerschbaumer.com>
date:       Wed Aug 19 10:43:30 2015 -0700
description:
Bug 1193552 - Remove baseURI from LoadInfo (r=sicking,jkitch)
https://hg.mozilla.org/mozilla-central/rev/495b6ed690b2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Depends on: 1211690
You need to log in before you can comment on or make changes to this bug.