Closed Bug 1180273 Opened 4 years ago Closed 4 years ago

newChannel2 callsite is undefined

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox40 + wontfix
firefox41 + wontfix
firefox42 + fixed

People

(Reporter: ursula, Assigned: ckerschb)

References

Details

Attachments

(1 file)

Bug 1147562 makes a call to newChannel2, however it returns undefined, since there's no reference to newChannel2 given. 
To fix this we should probably do something like this.newChannel2(aURI, null)
Flags: needinfo?(mozilla)
Blocks: 1021654
[Tracking Requested - why for this release]:

The patch in bug 1147562 appears to cause us to make several calls to an undefined newChannel2 function. That patch landed on 40.
(In reply to Ursula Sarracini (:ursula) from comment #0)
> Bug 1147562 makes a call to newChannel2, however it returns undefined, since
> there's no reference to newChannel2 given. 
> To fix this we should probably do something like this.newChannel2(aURI, null)

Ursula, I am happy to look into this problem immediately. In what file is that? If by any chance, please post an dxr link. Thanks!
Flags: needinfo?(mozilla) → needinfo?(ursulasarracini)
We should be fixing references to undefined functions. Adding a tracking flag for FF40, FF41 and FF42.
This is now wontfix for 40.

Andrew - Is this a bug for your team?
Flags: needinfo?(overholt)
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #5)
> 
> Andrew - Is this a bug for your team?

I don't think so as I think Christoph is handling it (based on comment 2).
Flags: needinfo?(overholt)
(In reply to Andrew Overholt [:overholt] from comment #6)
> (In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #5)
> > 
> > Andrew - Is this a bug for your team?
> I don't think so as I think Christoph is handling it (based on comment 2).

I'll have a look at this, do we have a testcase to reproduce as well?
Flags: needinfo?(ursulasarracini)
Hey Christoph,

No test case, no - we just noticed it from inspection. newChannel2 doesn't look like it's defined in the global scope.
Flags: needinfo?(ursulasarracini)
Gijs, you were helping me reviewing that change back in the days within Bug 1147562:
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/thumbnails/PageThumbsProtocol.js?from=pagethumbsprotocol.js#

Are we missing a 'this' and should it be:

> newChannel: function Proto_newChannel(aURI) {
> return this.newChannel2(aURI, null);
> }

If so, then we got that wrong in several other places as well:
http://mxr.mozilla.org/mozilla-central/search?string=return+newChannel2%28&find=.js&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Throughout Gecko we are (or at least should be) consistently be using newChannel2 on all callsites. Either way we should not return 'undefined' if someone is still using the deprecated 'newChannel' version and should get that fixed!
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> Gijs, you were helping me reviewing that change back in the days within Bug
> 1147562:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/thumbnails/
> PageThumbsProtocol.js?from=pagethumbsprotocol.js#
> 
> Are we missing a 'this'

It looks like it, yes. I don't know how I missed that when reviewing. Sorry. :-\
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #10)
> > Are we missing a 'this'
> 
> It looks like it, yes. I don't know how I missed that when reviewing. Sorry.
> :-\

No problem, I am on it.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Jonas, apparently we forgot to add a 'this' pointer in some cases when forwarding calls from newChannel to newChannel2 when updating those calls within Bug 1147562.

Gecko itself should not be using newChannel at all - do you think we still want to uplift those changes?
Attachment #8643312 - Flags: review?(jonas)
Comment on attachment 8643312 [details] [diff] [review]
bug_1180273_newChannel2_undefined.patch

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

Yes, we should probably uplift this. When did these protocols regress? I.e. which gecko train?
Attachment #8643312 - Flags: review?(jonas) → review+
(In reply to Jonas Sicking (:sicking) from comment #13)
> Yes, we should probably uplift this. When did these protocols regress? I.e.
> which gecko train?

Bug 1147562 landed in 40, so it will hit release soon.
I'm only worried about PageThumbsProtocol.js. The b2g ones doesn't matter because we don't have any products using 40.

But I have no idea what PageThumbsProtocol.js is or where it's used. Can you check with someone that knows that code?
Comment on attachment 8643312 [details] [diff] [review]
bug_1180273_newChannel2_undefined.patch

(In reply to Jonas Sicking (:sicking) from comment #15)
> But I have no idea what PageThumbsProtocol.js is or where it's used. Can you
> check with someone that knows that code?

Gijs, can you take a look at the toolkit changes here? Are you also for uplifting the patch?
Attachment #8643312 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8643312 [details] [diff] [review]
bug_1180273_newChannel2_undefined.patch

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

If gecko internally uses newChannel2 everywhere even on 40 then this shouldn't be too big a problem; it'd only be a problem if add-ons use the moz-page-thumb protocol in some kind of "manual" way. It seems to me like it would be more likely that they would use it directly as a URL for an image load or something like that, in which case it's up to gecko to make it work, AIUI.

Certainly it is risk-free enough that we could uplift pretty much anywhere - but 40rc1 has sailed and I don't know if we're respinning and how people would feel about uplift there. I would land on trunk asap, and request what uplift you can, and not worry too much until evidence to the contrary emerges.
Attachment #8643312 - Flags: review?(gijskruitbosch+bugs) → review+
I just checked the addons MXR, and I don't see anything there that looks like it would manually create a channel here (and only 2 add-ons, other than the fxos simulator, that use moz-page-thumb).
sorry had to back this out since one of this changes caused errors like xpcshell bustage and https://treeherder.mozilla.org/logviewer.html#?job_id=12554709&repo=mozilla-inbound
Flags: needinfo?(mozilla)
(In reply to Carsten Book [:Tomcat] from comment #20)
> sorry had to back this out since one of this changes caused errors like
> xpcshell bustage and
> https://treeherder.mozilla.org/logviewer.html#?job_id=12554709&repo=mozilla-
> inbound

This patch is actually not responsible for the problem - it's Bug 1191107, which landed together with this bug. The problem was that I didn't update the uuid. I will re-land that patch in soon-ish.
Flags: needinfo?(mozilla)
https://hg.mozilla.org/mozilla-central/rev/3a6ace7903cf
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
In bug 1192703 with this fix now present, we are seeing the following (custom) log trace that indicates that the MailtoProtocolHandler.js's (https://dxr.mozilla.org/mozilla-central/source/b2g/components/MailtoProtocolHandler.js) newChannel2 is now effectively being invoked twice.  Presumably this is because the newChannel2 does a side-effecty thing and then does "throw Components.results.NS_ERROR_ILLEGAL_VALUE;"  This is a common idiom for the b2g protocol handlers, ex: https://dxr.mozilla.org/mozilla-central/source/b2g/components/SmsProtocolHandler.js also throws.  (NB: As of right now, DXR is showing a stale tree state from 2015-08-07, ignore the fact that this patch is not applied.)

!!! mailto newChannel2
JavaScript error: , line 0: uncaught exception: 2147942487
!!! mailto newChannel deferring to newChannel2
!!! mailto newChannel2
JavaScript error: , line 0: uncaught exception: 2147942487
!!! shell.js creating activity: new from mail-handler
!!! shell.js creating activity: new from mail-handler
Okay, and nsIOService::NewChannelFromURIWithProxyFlagsInternal intentionally does this (https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIOService.cpp#757), so all the b2g protocol handlers are being dumb and need to turn their NewChannel implementations into no-ops or stop throwing errors.
(In reply to Andrea Marchesini (:baku) from bug 793310 comment 18)
> > This doesn't make sense to me. We send the async message, but then throw an
> > error?
> 
> We throw an exception because newChannel() must return a channel. But we do
> not want to create a channel: our propose is to send a async message to the
> shell.js. This code is taken from youtube protocol handler. Maybe sicking
> has some suggestion about this.

This seems to be the intent, but there was no discussion with jonas on the bug, so needinfoing now.  Jonas see comment 24 and comment 25 please.
Flags: needinfo?(jonas)
Ouch, I did not know that we had the side effects inside the newChannel implementations...

Would it be possible to move those into the AsyncOpen(2) functions instead?

Alternatively, it might work to have the newChannel function have the side effect, and let newChannel2 throw an exception. But I would prefer using AsyncOpen(2) instead.
Flags: needinfo?(jonas)
It looks like nsIWebBrowserChrome3[1] has a magical trick "shouldLoadURI" that does exactly what b2g wants, bailing out of the load process entirely very early[2].  The e10s stuff in browser.js seems to use this to redirect the load into e10s land[3].

It seem preferable to proceeding further down the "custom fake channel shenanigans" path or switching to the "nsIExternalProtocol*/nsIExternalApp*/nsIHelperApp*" path.  Maybe the biggest reason is that once we get beyond that nsIWebBrowserChrome3 logic, we end up at a point where the docshell will automatically stop network requests in that docshell for us[4].  Arguably clicking on a mailto link in a loading document shouldn't make the document stop loading, although that is, of course, how things do work in Firefox right now.

And another rationalization for shouldLoadURI is that by the time it's invoked, nsDocShell::InternalLoad has already processed aWindowTarget, locating and then handing the load off to another docshell instead of doing it itself.  Since our wacky protocol handlers do equally fancy, high-level things (trigger an activity that notifies an existing app or starts it up, which may involve spinning up a subprocess, etc.), arguably they are deserving an equally high-level bail-out.

The complicating factor would be mulet, since it probably does not want us becoming the mTreeOwner for the docshell and breaking everything in its entirety.  But that's why monkeypatching was invented, right?

In summary, ignore what I just said and let's do the easiest thing possible that makes the email app bug go away.


Refs:
1: nsIWebBrowserChrome3: https://dxr.mozilla.org/mozilla-central/source/embedding/browser/nsIWebBrowserChrome3.idl
2: its use by nsDocShell: https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10113
3: browser.js shouldLoadURI implementation: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4140
4: Stop(nsIWebNavigation::STOP_NETWORK); gets called at https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10194
(In reply to Jonas Sicking (:sicking) from comment #27)
> Would it be possible to move those into the AsyncOpen(2) functions instead?

And answering your actual question, the answer would seem to be yes, this is what other implementations do.  For example, nsExternalProtocolHandler has this cascade: AsyncOpen2 -> AsyncOpen -> OpenURL which then actually does nsIExternalProtocolService.LoadURI at https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalProtocolHandler.cpp#160 which I assume has a cool side effect.
I filed bug 1194087 to track fixing the b2g protocol handlers so this doesn't get lost.  (In case the see also spam didn't make it clear.)
No longer depends on: 1192703
See Also: → 1192703
Lawrence, I am still getting an email remainder about this bug. It's marked as a wontfix for 40 and since it's not causing any problems, I am suggesting we should also mark it as a wontfix for 41. All the discussion going on here after comment 24 is only B2G related. Please see also Comment 17 where Gijs explains why it should cause any problems.
Flags: needinfo?(lmandel)
Ack, thanks for the info.
Flags: needinfo?(lmandel)
You need to log in before you can comment on or make changes to this bug.