1362829 - Crash in nsChromeProtocolHandler::NewChannel2

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-f1c6caa9-7b7e-4047-b7bf-1e5f80170507.
=============================================================

Seen while looking at nightly crash stats - crashes started using 20170506030204: http://bit.ly/2p8SwAp

Possible regression range based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0b255199db9d6a6f189b89b7906f99155bde3726&tochange=37a5b7f6f101df2eb292b1b6baaf1540c9920e20

ni on honza since Bug 1319111 is in the range and maybe the cause?

Comment 1

[Honza Bambas (:mayhemer)]

Reproduced with [1] installed and visiting [2].  JS stack:

0 newChannel(aURI = [xpconnect wrapped (nsISupports, nsIStandardURL, nsIURI) @ 0xd297730 (native @ 0x139a354)]) ["jar:file:///c:/Mozilla/profiles/FX1/extensions/%7B86095750-AD15-46d8-BF32-C0789F7E6A32%7D.xpi!/data/js/frame-script.js":101]
    this = [object Object]
1 restoreTabContent(loadArguments = [object Object], isRemotenessUpdate = true, finishCallback = () => {
      // Tell SessionStore.jsm that it may want to restore some more tabs,
      // since it restores a max of MAX_CONCURRENT_TAB_RESTORES at a time.
      sendAsyncMessage("SessionStore:restoreTabContentComplete", {epoch, isRemotenessUpdate});
    }) ["resource:///modules/sessionstore/ContentRestore.jsm":216]
    this = [object Object]
2 bound restoreTabContent([object Object], true, () => {
      // Tell SessionStore.jsm that it may want to restore some more tabs,
      // since it restores a max of MAX_CONCURRENT_TAB_RESTORES at a time.
      sendAsyncMessage("SessionStore:restoreTabContentComplete", {epoch, isRemotenessUpdate});
    }) ["self-hosted":950]
    this = [object Object]
3 restoreTabContent((destructured parameter) = [object Object]) ["chrome://browser/content/content-sessionStore.js":221]
    this = [object Object]
4 receiveMessage((destructured parameter) = [object Object]) ["chrome://browser/content/content-sessionStore.js":155]
    this = [object Object]

Loading chrome://bbsfox/content/telnet.html as a result of docshell top level load of telnet://ptt.cc/.


Boris, should we fail the chrome load (throw from newChannel2) when there is no loadinfo provided?


[1] https://addons.cdn.mozilla.net/user-media/addons/179388/bbsfox-5.0.22-fx.xpi?filehash=sha256%3A3cf5e7e27c6060532718fdfabe2022bce6a0f687e5969d08b371f767d3950510
[2] telnet://ptt.cc/

Comment 2

[Honza Bambas (:mayhemer)]

Ah.. the handler in frame-script.js for telnet has to implement newChannel2 to carry the load info down (call on it throws NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED).

So I think we can throw from the chrome handler and let the addon author fix this.  Swu, could you please help here to reach (find a way to reach) the extension developer?  It's a mozilla taiwan forum.

The amo site: https://addons.mozilla.org/en-US/firefox/addon/bbsfox/?src=search

Thanks.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Boris, should we fail the chrome load (throw from newChannel2) when there is no loadinfo provided?

Yes!  This was a review comment in bug 1319111 comment 83.  Quoting:

  One overall problem I just realized: in a bunch of places where we're setting
  resultPrincipalURI we might have a null loadinfo.  It's probably OK to just throw
  from newChannel in those case, so the JS ones need no changes, but the C++ ones
  need to null-check and either throw or silently not set resultPrincipalURI.

Did that review comment not get addressed?

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #3)
> > Boris, should we fail the chrome load (throw from newChannel2) when there is no loadinfo provided?
> 
> Yes!  This was a review comment in bug 1319111 comment 83.  Quoting:
> 
>   One overall problem I just realized: in a bunch of places where we're
> setting
>   resultPrincipalURI we might have a null loadinfo.  It's probably OK to
> just throw
>   from newChannel in those case, so the JS ones need no changes, but the C++
> ones
>   need to null-check and either throw or silently not set resultPrincipalURI.
> 
> Did that review comment not get addressed?

I definitely missed the part to add proper nullchecks.  However, there are two ways we handle the missing loadinfo after the patch:

1. SubstitutingProtocolHandler, nsViewSourceHandler, WyciwygChannelParent have null checks, but when load info is null, we don't modify it and go on with NS_OK.

2. (ns)AboutRedirector, nsHostObjectProtocolHandler, nsChromeProtocolHandler are missing null checks.


So, for consistency, all these should have NS_ENSURE_STATE(loadinfo).

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: v1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3150bfd2d5364adde64916f36bad047a23e73c0d

Comment 6

[Shian-Yow Wu [:swu]]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> Ah.. the handler in frame-script.js for telnet has to implement newChannel2
> to carry the load info down (call on it throws
> NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED).
> 
> So I think we can throw from the chrome handler and let the addon author fix
> this.  Swu, could you please help here to reach (find a way to reach) the
> extension developer?  It's a mozilla taiwan forum.
> 
> The amo site:
> https://addons.mozilla.org/en-US/firefox/addon/bbsfox/?src=search
> 
> Thanks.

Thanks Honza for this info.  I've left a note in the forum[1], so the author as well as add-on users will get notified of this. 

https://forum.moztw.org/viewtopic.php?f=11&t=30217&p=190336#p190336

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Shian-Yow Wu [:swu] from comment #6)
> Thanks Honza for this info.  I've left a note in the forum[1], so the author
> as well as add-on users will get notified of this. 

Thank you!

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #5)
> Created attachment 8865509 [details] [diff] [review]
> v1
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3150bfd2d5364adde64916f36bad047a23e73c0d

Try seems to be green, all the oranges are tracked and compared to the base change set, the orange rate seems similar.

Comment 9

[avada]

I don't understand all this talk about an addon. An addon is definitely not at fault here. I've been using the same profile for ages, but it only started to crash a few days ago. Every single time I open about:addons.

Here are some of my earliest crash reports:
https://crash-stats.mozilla.com/report/index/f51aedeb-500b-43e2-8544-177860170507
https://crash-stats.mozilla.com/report/index/a05f6024-754a-4f00-bce4-1fe1a0170507

Comment 10

[Boris Zbarsky [:bzbarsky]]

An addon is doing something that, due to changes to core code, is causing the core code to crash.  The core code will be changed to fix the crash (by refusing to do what the addon is asking it to do).  However, the addon will then likely need to be fixed to actually work correctly.

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 8865509 [details] [diff] [review]
v1

We probably won't need this patch.  Bug 1319111 is gonna be fixed in a different way.

Comment 12

[avada]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #10)
> An addon is doing something that, due to changes to core code, is causing
> the core code to crash.  The core code will be changed to fix the crash (by
> refusing to do what the addon is asking it to do).  However, the addon will
> then likely need to be fixed to actually work correctly.

I see. Any practical way to figure out which addon (if any) prompts the crash?

Or can I expect a usable error message when this bugfix lands?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Per comment 11, we're going to try something that preserves addon compat better here.  So there's a good chance nothing will be needed on the addon side if that works out.

Comment 14

[ettoolong]

Hi, I am the BBSFox author.

Thanks swu for this info.

I can't get aLoadInfo in newChannel, right?
I try to modify my code, replace newChannelFromURIWithLoadInfo with newChannelFromURI2
Here is my code:
https://github.com/ettoolong/BBSFox-Beta/commit/87980a4b7505a8117f9cca3e461201518813f049
It break everything. :(

The protocol handler redirect telnet://ptt.cc to chrome://bbsfox/content/telnet.html
and the URL bar display: "jar:file:///var/folders/..../bc6e507b-af55-4c8a-9caf-03c1ade3880b-copy/extensions/%7B86095750-AD15-46d8-BF32-C0789F7E6A32%7D.xpi!/chrome/content/telnet.html"
My javascript in telnet.html that use document.location.hostname and document.location.port to create my TCP socket. I can't get these information any more.
(In previous Firefox version, URL bar shows: telnet://ptt.cc, document.location.hostname=ptt.cc)

About Bug 1319111, I still need any modify in my add-on?

BTW, I also test the web-ext protocol handler ( in Firefox 54 ),
It redirect ssh://ptt.cc to moz-extension://.../ssh.html
the URL bar display "moz-extension://.../ssh.html" not "ssh://ptt.cc"
Add-on user may feel confuse. Why URL change to "moz-extension://.../ssh.html"?

Can I keep "telnet://host:port" or "ssh://host:port" in URL bar after Firefox 55?

Comment 15

[Shian-Yow Wu [:swu]]

Ni? Honza for comment 14.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> I can't get aLoadInfo in newChannel, right?

That's correct.  But you get aLoadInfo in newChannel2 if you implement it.

> About Bug 1319111, I still need any modify in my add-on?

Unclear at the moment.  Once we sort out whether we can do a more addon-compatible fix here, we'll follow up.  Sorry for the hassle, and thanks for being on top of things!

> Can I keep "telnet://host:port" or "ssh://host:port" in URL bar after Firefox 55?

This, I don't know.  Kris, do you know?

Comment 17

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #16)
> > Can I keep "telnet://host:port" or "ssh://host:port" in URL bar after Firefox 55?
> 
> This, I don't know.  Kris, do you know?

After 55, yes. After 57, no, not as of this moment. Bug 1362829 would allow that, but it's unlikely to be implemented in time for 55, if at all. Bug 1310427 should allow implementing handlers for those protocols, but they'd have to redirect to a different URL, which would be visible in the URL bar.

Comment 18

[Wayne Mery (:wsmwk)]

One Thunderbird users states his crash is caused by custombuttons addon. 
Others have reported same for Firefox http://custombuttons.sourceforge.net/forum/viewtopic.php?f=5&t=12108

Comment 19

[Marcia Knous [:marcia]]

(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #18)
> One Thunderbird users states his crash is caused by custombuttons addon. 
> Others have reported same for Firefox
> http://custombuttons.sourceforge.net/forum/viewtopic.php?f=5&t=12108

Thanks Wayne - I was able to reproduce it be installing the addon and then restarting Nightly on Mac.

Comment 20

[Honza Bambas (:mayhemer)]

I'm going to back bug 1319111 out now.  It will fix this crash.  A newer version of a fix for that bug will have less impact on addons and won't cause these types of crashes.

Comment 21

[Worcester12345]

(In reply to Honza Bambas (:mayhemer) from comment #20)
> I'm going to back bug 1319111 out now.  It will fix this crash.  A newer
> version of a fix for that bug will have less impact on addons and won't
> cause these types of crashes.

Was this backout done? Did it work? Should this then be changed to "WFM" (Works For Me), not "WONTFIX"?