Closed Bug 707624 Opened 13 years ago Closed 1 year ago

e10s HTTP: we don't support redirects from HTTP -> data: URIs

Categories

(Core :: Networking: HTTP, defect, P3)

defect

Tracking

()

RESOLVED WORKSFORME
Tracking Status
e10s later ---

People

(Reporter: jduell.mcbugs, Unassigned)

References

Details

(Whiteboard: [necko-backlog])

I just noticed that e10s doesn't support redirecting from HTTP to a data: URI.  The parent channel constructs the new Data channel fine, and registers an ID for it, and  HttpChannelChild::Redirect1Begin creates a new data channel fine, too (when it tries to QI it to nsIChildChannel that fails and NS_ERROR is called, but that's just a warning).  I traced the code all the way through nsAsyncRedirectVerifyHelper.cpp up to the DocShell's redirect handler, and it all seemed fine, but the browser just stays on the old page.  Not sure where it's going wrong, and don't have time to find out.

This has come up while trying to answer a question about how to fix 354493, i.e. not from a bug in the field.  But I expect we'll hit it at some point--it's a legal redirect.

Also, presumably even if we get this working in the child, we'll have a memory leak in the parent? (Since we register the redirected-to data channel but don't ever use it).  Maybe we should special-case data, since all the info is in the URI, and we don't need a channel on the parent (except to pass to redirect handlers).

I have a test URI at

   http://people.mozilla.org/~jduell/data   

(you should see "foobar" after going there)

Honza: does bug 590682 fix this?
Hmm, I now notice that entering a plain old "data:,fooooo" into fennec (i.e. no redirect involved) doesn't seem to work, so perhaps the issue here is fennec not supporting the data protocol.

cc-ing mobile guys in case they're not aware of this or can fill me in on why data: isn't supported.
Comment 0 is a duplicate of bug 661604 I believe, which I found a while ago when doing my big mochitest push.
(In reply to Jason Duell (:jduell) from comment #0)
> Honza: does bug 590682 fix this?

No, because for data: we create the actual handling channel on the child process, data handler sets the DONT_OVERRIDE_ON_CHILD_PROCESS flag in that patch, at [1] we will get NULL.

I don't think it's a good idea at all to let data be processed on the parent process, the actual data would have been carried between processes here and there w/o any obvious need, not quit optimal, IMO.  Therefor I've set the flag for data: in that patch.


I have an idea: the patch in bug 590682 expects to do the actual processing on the parent.  That is a valid use case.  But we may need a second generic pair that expects to handle on the child process!  Actually an opposite addition to what that patch currently implements.  We will just add one more flag to nsIProtocolHandler and condition to IO service to create a generic channel (not IPC) when creating the new redirect channel.  When we get NULL at [1], we can create a generic channel "manually" and let it connect with the fake channel on the parent, using the existing channel registrar.  

In other words: the patch in bug 590682 is now incomplete!


Another idea, which I'm putting here just for a reference (complicated): We may have to think about redirects ones again.  Maybe creating the actual channel implementation on the parent process as new channel we redirect to was not a good idea at all.  Maybe we have to post an event to the child process and let it create the pair it self and than catch it on the parent asynchronously.  No need for the channel registrar.  Just need to let the redirecting channel wait for the new channel asynchronously.  The generic channel from bug 590682 can be rethinked to create a generic parent channel for e.g. data: and other channels that will process on child and don't want to implement the IPC part just because of http can redirect to that protocol.



I don't know at the moment why you cannot simply open a data: URL.  Not enough time at the moment to test this my self.


[1] http://hg.mozilla.org/mozilla-central/annotate/dfff3e59ef23/netwerk/protocol/http/HttpChannelChild.cpp#l770
If bug 786275 then we don't have to fix this...
Depends on: 786275
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #4)
> If bug 786275 then we don't have to fix this...

This isn't quite true, in bug 1146132 an add-on author is redirecting http requests to a data URI in order to replace the page with something else.
Whiteboard: [necko-backlog]
See Also: → 590682
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: P1 → P3
Would this apply to other types of non-HTTP URLs also?

Bug 1416146 is about redirecting from HTTP to extension URLs ("moz-extension" scheme) in extension code.
Flags: needinfo?(jduell.mcbugs)
> Would this apply to other types of non-HTTP URLs also?

We don't support redirects to extension URIs right now.  I'm not actually sure of what our webextension APIs are for extension URLS, but whatever they are, I doubt we support them.  

But support for that would probably be a different bug than this one (although the fix might be related).  Open a new bug for it if Andy McKay thinks we need to support it.
Flags: needinfo?(jduell.mcbugs)
(In reply to Jason Duell [:jduell] (needinfo me) from comment #10)
> > Would this apply to other types of non-HTTP URLs also?
> 
> We don't support redirects to extension URIs right now.  I'm not actually
> sure of what our webextension APIs are for extension URLS, but whatever they
> are, I doubt we support them.  

We do support redir to moz-extension: (bug 1276048 and its dup).  My work on bug 1319111 was one of the blockers to make this actually work.  Not sure in what state it is now, but we allow it only with few corner cases unfixed?

> 
> But support for that would probably be a different bug than this one
> (although the fix might be related).  Open a new bug for it if Andy McKay
> thinks we need to support it.

I don't know why we should not support redir to data unless there is some redirect/ipc magic that is missing (nsIChildChannel/nsIParentChannel impl for data channels.)

Also note there were some limitations added to do top level loads from data (still evolving, tho) that could potentially affect this.
(In reply to Honza Bambas (:mayhemer) from comment #11)
> We do support redir to moz-extension: (bug 1276048 and its dup).  My work on
> bug 1319111 was one of the blockers to make this actually work.  Not sure in
> what state it is now, but we allow it only with few corner cases unfixed?

Thanks Honza! Sounds like it has since regressed. I'll poke those folks about fixing the regression.
Severity: normal → S3

this was fixed at some point, and now in bug 1691658 we just deliberately broke the non-extension cases (though with a pref in case we break the world). So I think this is effectively somewhere between wfm and wontfix.

Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.