Crash in nsChromeProtocolHandler::NewChannel2

RESOLVED FIXED

Status

()

Core
Security: CAPS
--
critical
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: marcia, Assigned: mayhemer)

Tracking

({crash, regression, reproducible})

55 Branch
Unspecified
Windows 10
crash, regression, reproducible
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 fixed)

Details

(Whiteboard: [tbird crash] [fixed by patch backout in bug #1319111], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

10 months ago
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?
Flags: needinfo?(odvarko)
Flags: needinfo?(odvarko) → needinfo?(honzab.moz)
(Assignee)

Comment 1

10 months ago
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/
Assignee: nobody → honzab.moz
Flags: needinfo?(honzab.moz) → needinfo?(bzbarsky)
(Assignee)

Comment 2

10 months ago
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.
Flags: needinfo?(swu)
> 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?
Blocks: 1319111
Flags: needinfo?(bzbarsky) → needinfo?(honzab.moz)
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox55: --- → affected
status-firefox-esr52: --- → unaffected
(Assignee)

Comment 4

10 months ago
(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).
Flags: needinfo?(honzab.moz)

Comment 6

10 months ago
(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
Flags: needinfo?(swu)
(Assignee)

Comment 7

10 months ago
(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!
(Assignee)

Comment 8

10 months ago
(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

10 months ago
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
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.
(Assignee)

Comment 11

10 months ago
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.
Attachment #8865509 - Flags: review?(bzbarsky)

Comment 12

10 months ago
(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?
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

10 months ago
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

10 months ago
Ni? Honza for comment 14.
Flags: needinfo?(honzab.moz)
> 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?
Flags: needinfo?(honzab.moz) → needinfo?(kmaglione+bmo)
(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.
Flags: needinfo?(kmaglione+bmo)

Comment 18

10 months ago
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
Whiteboard: [tbird crash]
(Reporter)

Comment 19

10 months ago
(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.
Keywords: reproducible
(Assignee)

Comment 20

10 months ago
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.
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → WONTFIX

Comment 21

9 months ago
(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"?
Flags: needinfo?(honzab.moz)
status-firefox55: affected → fixed
Flags: needinfo?(honzab.moz)
Resolution: WONTFIX → FIXED
Whiteboard: [tbird crash] → [tbird crash] [fixed by patch backout in bug #1319111]
You need to log in before you can comment on or make changes to this bug.