Closed
Bug 1259511
Opened 8 years ago
Closed 8 years ago
Add deprecation warning for addon devs to indicate that nsIAboutModule newchannel() must set loadinfo
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: dholbert, Assigned: ckerschb)
References
Details
(Keywords: addon-compat, Whiteboard: [domsecurity-active][issue w/ stylish being fixed via add-on update])
Attachments
(2 files, 1 obsolete file)
15.79 KB,
text/plain
|
Details | |
3.54 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•8 years ago
|
||
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.
Reporter | ||
Comment 2•8 years ago
|
||
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)
Flags: needinfo?(mozilla)
Reporter | ||
Comment 3•8 years ago
|
||
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.
Reporter | ||
Updated•8 years ago
|
Summary: "Stylish" add-on doesn't work in Nightly → "Stylish" add-on doesn't work in Nightly (its editing UI, "about:stylish-edit", is just a blank page)
Reporter | ||
Comment 4•8 years ago
|
||
[CC'ing Jason Barnabe, the Stylish maintainer]
Assignee | ||
Comment 5•8 years ago
|
||
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
Flags: needinfo?(mozilla)
Reporter | ||
Comment 6•8 years ago
|
||
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.
Reporter | ||
Updated•8 years ago
|
Component: DOM: Security → Add-ons
Product: Core → Tech Evangelism
Reporter | ||
Comment 7•8 years ago
|
||
(I now see this was brought up a bit in bug 1254752, too, e.g. bug 1254752 comment 13 & bug 1254752 comment 16. Cool.)
Assignee | ||
Comment 8•8 years ago
|
||
Jonas, as discussed on IRC we should log a warning to the console and make that part backwards compatible so addons still work.
Attachment #8734541 -
Flags: review?(jonas)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Component: Add-ons → DOM: Security
Product: Tech Evangelism → Core
Whiteboard: [domsecurity-active]
Assignee | ||
Comment 9•8 years ago
|
||
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 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
Attachment #8734541 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8734541 -
Attachment is obsolete: true
Attachment #8734549 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 12•8 years ago
|
||
So is the new Stylish code correct and not deprecatey? https://github.com/JasonBarnabe/stylish/commit/f50999d33ace7c6749d14e3108e6eab912b8b31d
Assignee | ||
Comment 13•8 years ago
|
||
(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•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/374ac7d2b3e3
Keywords: checkin-needed
Reporter | ||
Comment 15•8 years ago
|
||
(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.)
Flags: needinfo?(mozilla)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(mozilla)
Summary: "Stylish" add-on doesn't work in Nightly (its editing UI, "about:stylish-edit", is just a blank page) → Add deprecation warning for addon devs to indicate that nsIAboutModule newchannel() must set loadinfo
Reporter | ||
Updated•8 years ago
|
Whiteboard: [domsecurity-active] → [domsecurity-active][issue w/ stylish being fixed via add-on update]
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/374ac7d2b3e3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
Keywords: addon-compat
You need to log in
before you can comment on or make changes to this bug.
Description
•