1259511 - Add deprecation warning for addon devs to indicate that nsIAboutModule newchannel() must set loadinfo

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Install Stylish from
   https://addons.mozilla.org/en-US/firefox/addon/stylish/
 2. Restart Firefox to complete installation.
 3. Try to author a style, by visiting "about:stylish-edit" (or by clicking its toolbar icon & choosing Write new Style | Blank Style)

ACTUAL RESULTS:
A blank tab is loaded.

EXPECTED RESULTS:
The stylish "edit style" UI should be loaded in a tab.

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3ca8a82bbff8c9f6a04451b8a9452a1213f5e8fa&tochange=25a61e969b4ea97f727d5711577a826a9733261a

This seems similar to bug 1256755 (breakage with BrowserStack add-on, with the same regression range), except that the BrowserStack add-on went back to working after bug 1257339 landed, whereas Stylish is still broken.

Comment 1

[Daniel Holbert [:dholbert]]

I get this output in the error console when I perform STR in current Nightly:
Warning: 'nsIOService::NewChannel()' deprecated, please use 'nsIOService::NewChannel2()'
Source link: aboutStylishEdit.js:21:16

[Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIWebNavigation.loadURIWithOptions]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://browser/content/browser.js :: _loadURIWithFlags :: line 841"  data: no]
Source link: (unknown)


I think that latter exception is what's causing trouble here.

Comment 2

[Daniel Holbert [:dholbert]]

If I toggle the little open/close arrow next to that exception, it gives me this backtrace:

_loadURIWithFlags()                        browser.js:841
loadURIWithFlags()                         tabbrowser.xml:6704
addTab()                                   tabbrowser.xml:1979
loadOneTab()                               tabbrowser.xml:1472
stylishCommon.openEdit()                   common.js:106
stylishCommon.addCode()                    common.js:324
stylishOverlay.addCode()                   overlay.js:457
stylishOverlay.writeStylePopupShowing/<()  overlay.js:215

The innermost stylish code there ("openEdit") pointing to this line of code:
> win.gBrowser.loadOneTab(url, {inBackground: false, relatedToCurrent: true});
though I'm not sure there's much we can learn from that.

ckerschb, I'm hoping you can take a look at this and see what might be going wrong. (& suggest a fix for Stylish if necessary)

Comment 3

[Daniel Holbert [:dholbert]]

Sorry, I just noticed -- I only get that [Exception] if I load the URL via Stylish's toolbar icon.

If I just directly try to visit "about:stylish-edit" by typing that into my URL bar and hitting enter, then I only get the NewChannel deprecation warning in the error console.

Comment 4

[Daniel Holbert [:dholbert]]

[CC'ing Jason Barnabe, the Stylish maintainer]

Comment 5

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: stacktrace_stylish_bug.txt

I am not 100% sure because I don't have the source of 'stylish' at hand, but I suppose stylish implements nsIAboutModule, which newChannel() signature changed a while ago [1]. What stylish would have to do is update |newChannel: function(aURI)| to:

newChannel: function(aURI, aLoadInfo) {
  ...
  var channel = ios.newChannelFromURIWithLoadInfo(whatevernewURI, aLoadInfo);
  ...
  return channel;
}

The reason this is not working anymore after Bug 1257339 landed is that we provide default loadInfo arguments for NewChannel2() within nsIOService->NewChannel() [2]. Attaching those loadinfo arguments in turn enters the following if-branch [3] which causes the assertion to get triggered (see attached stacktrace).

Question in general, how many addons implement nsIAboutModule? If there are more, then we should also revert the change within [2] and pass nullptr as the loadingPrincipal instead of the systemPrincipal.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsIAboutModule.idl#22
[2] https://hg.mozilla.org/integration/mozilla-inbound/rev/c31503dab99d#l4.92
[3] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIOService.cpp#803

Comment 6

[Daniel Holbert [:dholbert]]

Looks like ckerschb is correct about nsIAboutModule & newChannel, and it turns out Jason has already addressed this on the github stylish repo:
 https://github.com/JasonBarnabe/stylish/issues/274

So I suspect this will become FIXED once that change makes it to a Stylish release.

Comment 7

[Daniel Holbert [:dholbert]]

(I now see this was brought up a bit in bug 1254752, too, e.g. bug 1254752 comment 13 & bug 1254752 comment 16. Cool.)

Comment 8

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1259511_aboutprotocolhandler_loadinfo_update.patch

Jonas, as discussed on IRC we should log a warning to the console and make that part backwards compatible so addons still work.

Comment 9

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Now that we have a patch for Gecko, I think we should revert the flags to keep track of that bug within DOM:Security.

Comment 10

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

Comment on attachment 8734541 [details] [diff] [review]
bug_1259511_aboutprotocolhandler_loadinfo_update.patch

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

r=me with that fix.

::: netwerk/protocol/about/nsAboutProtocolHandler.cpp
@@ +186,5 @@
>              // an interim solution we set the LoadInfo here if not
>              // available on the channel. Bug 1087720
> +            nsCOMPtr<nsILoadInfo> loadInfo = (*result)->GetLoadInfo();
> +            if (aLoadInfo != loadInfo) {
> +              NS_ASSERTION(false, "nsIAboutModule->newChannel(aURI, aLoadInfo) needs to set LoadInfo");

The indentation here is wrong. This file seems to use 4-space indentation.

Also, we should only assert and report if loadInfo != nullptr

Comment 11

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1259511_aboutprotocolhandler_loadinfo_update.patch

Comment 12

[Jason Barnabe (np)]

So is the new Stylish code correct and not deprecatey?

https://github.com/JasonBarnabe/stylish/commit/f50999d33ace7c6749d14e3108e6eab912b8b31d

Comment 13

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Jason Barnabe (np) from comment #12)
> So is the new Stylish code correct and not deprecatey?
> 
> https://github.com/JasonBarnabe/stylish/commit/
> f50999d33ace7c6749d14e3108e6eab912b8b31d

That code is absolutely correct!

JFYI, basically the only thing we are performing within this bug is adding a deprecation warning so addon authors know that they soon have to update their code because NewChannel() will be deprecated.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/374ac7d2b3e3

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #13)
> JFYI, basically the only thing we are performing within this bug is adding a
> deprecation warning

You should probably morph the title of this bug (to "Add warning about [...]") then, since it'll soon be RESOLVED|FIXED without the issue described in the summary actually having been fixed.  (The issue in the summary will be fixed via the next stylish update, presumably.)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/374ac7d2b3e3