1386533 - HttpObserverManager.canModify throws breaking WaybackMachine add-on

Status: VERIFIED FIXED

(WebExtensions :: Request Handling, defect, P1)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

With the WaybackMachine extension enabled (1.8.1) latest nightly breaks opening a new tab with the following log in the console:

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.host]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/WebRequest.jsm :: canModify :: line 852"  data: no] (unknown)
	canModify resource://gre/modules/WebRequest.jsm:852:38
	runChannelListener resource://gre/modules/WebRequest.jsm:883:23
	applyChanges resource://gre/modules/WebRequest.jsm:1002:15
	next self-hosted:1183:9
tab is undefined  background.js:49
Unchecked lastError value: Error: Invalid tab ID: -1  ExtensionCommon.jsm:304

My guess is that this code here needs to put a try/catch around loadingPrincipal.URI.host which throws in about: pages:

http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/toolkit/modules/addons/WebRequest.jsm#852

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1386533 fix host permitted matching,

Review commit: https://reviewboard.mozilla.org/r/164126/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164126/

Comment 2

[Shane Caraveo (:mixedpuppy)]

[Tracking Requested - why for this release]: A small fix for an issue that could affect a lot of extensions.

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8893127 [details]
Bug 1386533 fix host permitted matching,

https://reviewboard.mozilla.org/r/164126/#review169528

Comment 4

[Gerry Chang [:gchang]]

Track 56+ as the impact on lots of extensions.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28f22d3e5ce1
fix host permitted matching, r=kmag

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8893127 [details]
Bug 1386533 fix host permitted matching,

Making an early request, this is a simple fix that can be uplifted.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1308688
[User impact if declined]: some request monitoring fails
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: just landed
[Needs manual test from QE? If yes, steps to reproduce]: install wayback machine addon and use to view recent versions of a webpage.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: It's just a catch around nsIURI.host access which throws for non-host URIs such as "about:"
[String changes made/needed]: none

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/28f22d3e5ce1

Comment 8

[Aki Sasaki (not active)]

https://hg.mozilla.org/projects/date/rev/28f22d3e5ce131ca1fada15226163028a39043a6
Bug 1386533 fix host permitted matching, r=kmag

Comment 9

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8893127 [details]
Bug 1386533 fix host permitted matching,

Small fix that may have a good impact on many webextensions.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/25aa08f84e10

Comment 11

[Ryan VanderMeulen [:RyanVM]]

I had to back this out from Beta for causing frequent NS_ERROR_SOCKET_ADDRESS_IN_USE failures on ASAN mochitest-bc runs. I confirmed this patch was the culprit after running the backout through Try and doing mass retriggers to confirm.

https://hg.mozilla.org/releases/mozilla-beta/rev/667773e6e8c1

Here's a sampling of what the failures looked like:
https://treeherder.mozilla.org/logviewer.html#?job_id=122289586&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=122291164&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=122289321&repo=try

I had initially reported them under bug 1389063, but the frequency was high enough that I didn't feel comfortable leaving the patch in without further investigation.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

*sigh* 160 green runs on Try, but still hitting the failure on Beta :(. Re-landed and apologies for the churn.

https://hg.mozilla.org/releases/mozilla-beta/rev/2638feb177dd9104a3887fae2ce05931a6b41d6c

Comment 13

[marius.santa]

Attachment: version check.gif

This bug is verified on Firefox 56.0 (20170926190823) and Firefox 57.0b8 (20171013042429) under Windows 10 64bit/Mac OS X 10.12.3.

Please see the attached video.