1180273 - newChannel2 callsite is undefined

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Ursula Sarracini (:ursula)]

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)

Comment 1

[Mike Conley (:mconley) (:⚙️)]

[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.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

(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!

Comment 3

[Ursula Sarracini (:ursula)]

Thanks so much!
Here's the DXR link: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/thumbnails/PageThumbsProtocol.js?from=pagethumbsprotocol.js#88 

:)

Comment 4

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

We should be fixing references to undefined functions. Adding a tracking flag for FF40, FF41 and FF42.

Comment 5

[Lawrence Mandel [:lmandel] (use needinfo)]

This is now wontfix for 40.

Andrew - Is this a bug for your team?

Comment 6

[Andrew Overholt [: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).

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(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?

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Hey Christoph,

No test case, no - we just noticed it from inspection. newChannel2 doesn't look like it's defined in the global scope.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

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!

Comment 10

[:Gijs (he/him)]

(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. :-\

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1180273_newChannel2_undefined.patch

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?

Comment 13

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

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?

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 15

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

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 16

[Christoph Kerschbaumer [:ckerschb]]

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?

Comment 17

[:Gijs (he/him)]

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.

Comment 18

[:Gijs (he/him)]

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).

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d98d82f9981b

Comment 20

[Carsten Book [:Tomcat]]

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

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a6ace7903cf

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3a6ace7903cf

Comment 24

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 25

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 26

[Andrew Sutherland [:asuth] (he/him)]

(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.

Comment 27

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

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.

Comment 28

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 29

[Andrew Sutherland [:asuth] (he/him)]

(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.

Comment 30

[Andrew Sutherland [:asuth] (he/him)]

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.)

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 32

[Sylvestre Ledru [:Sylvestre]]

Ack, thanks for the info.