Implement webRequest.onErrorOccurred

RESOLVED FIXED in Firefox 48

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: josesigna, Assigned: mao)

Tracking

(Blocks: 1 bug)

47 Branch
mozilla48
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [webRequest] triaged, URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.116 Safari/537.36

Steps to reproduce:

Running a WebExtensions extension and using "webRequest.onErrorOcurred" triggers a warning on the Browser Console:
"In add-on firefox@ghostery.com, attempting to use listener "webRequest.onErrorOccurred", which is unimplemented."



Expected results:

But it's listed as supported here http://arewewebextensionsyet.com/
(Reporter)

Updated

2 years ago
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86
(Reporter)

Updated

2 years ago
Flags: blocking-webextensions+

Updated

2 years ago
Assignee: nobody → g.maone
Flags: blocking-webextensions+ → blocking-webextensions?

Updated

2 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

2 years ago
Blocks: 1213483
Status: NEW → ASSIGNED
Summary: webRequest.onErrorOccurred listed as implemented but it's not → Implement webRequest.onErrorOccurred
(Assignee)

Updated

2 years ago
Blocks: 1235639
(Assignee)

Updated

2 years ago
Iteration: --- → 48.1 - Mar 21
(Assignee)

Updated

2 years ago

Updated

2 years ago
Flags: blocking-webextensions? → blocking-webextensions+
Whiteboard: [webRequest] triaged
(Assignee)

Comment 1

a year ago
Created attachment 8731685 [details]
MozReview Request: Bug 1252596 - Implement webRequest.onErrorOccurred. r?kmag

Review commit: https://reviewboard.mozilla.org/r/40775/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40775/
Attachment #8731685 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 2

a year ago
Comment on attachment 8731685 [details]
MozReview Request: Bug 1252596 - Implement webRequest.onErrorOccurred. r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40775/diff/1-2/
Comment on attachment 8731685 [details]
MozReview Request: Bug 1252596 - Implement webRequest.onErrorOccurred. r?kmag

https://reviewboard.mozilla.org/r/40775/#review38295

Sorry this took so long. I didn't get a chance last week, and I've been traveling.

::: toolkit/components/extensions/test/mochitest/file_WebRequest_page1.html:30
(Diff revision 2)
>  
>  <iframe src="file_WebRequest_page2.html" width="200" height="200"></iframe>
>  <iframe src="redirection.sjs" width="200" height="200"></iframe>
>  <iframe src="data:text/plain,webRequestTest" width="200" height="200"></iframe>
> -
> +<iframe src="data:text/plain,webRequestTest_bad" width="200" height="200"></iframe>
> +<iframe src="https://invalid.mozilla.com/" width="200" height="200"></iframe>

Mochitests aren't allowed to make requests to external servers. Please choose one of the hosts listed in https://dxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest.html:305
(Diff revision 2)
>          browser.test.assertTrue(details.frameId != 0, "sub-frame gets its own frame ID");
>          browser.test.assertTrue(details.frameId !== undefined, "sub-frame ID defined");
>          browser.test.assertEq(details.parentFrameId, 0, "parent frame id is correct");
>        }
>      }
> -    if (details.url.indexOf("_bad.") != -1) {
> +    if (details.url.indexOf("_bad") != -1) {

`if (details.url.includes("_bad"))`

::: toolkit/modules/addons/WebRequest.jsm:102
(Diff revision 2)
>      channel.QueryInterface(Ci.nsIHttpChannelInternal).getResponseVersion(maj, min);
>      data.statusLine = `HTTP/${maj.value}.${min.value} ${data.statusCode} ${statusText}`;
>    } catch (e) {
> -    // NS_ERROR_NOT_AVAILABLE might be thrown.
> -    Cu.reportError(e);
> +    // NS_ERROR_NOT_AVAILABLE might be thrown if it's an internal redirect, happening before
> +    // any actual HTTP traffic. Otherwise, let's report.
> +    if (event !== "onRedirect") {

Shouldn't this be `if (event !== "onRedirect" && e.result !== Cr.NS_ERROR_NOT_AVAILABLE)`?

::: toolkit/modules/addons/WebRequest.jsm:442
(Diff revision 2)
> +    let prefix = /^(?:ACTIVITY_SUBTYPE_|STATUS_)/;
> +    let map = new Map();
> +    for (let iface of [Ci.nsIHttpActivityObserver, Ci.nsISocketTransport]) {
> +      for (let c of Object.keys(iface).filter(name => prefix.test(name))) {
> +        let value = iface[c];
> +        this[c.replace(prefix, "ACTIVITY_")] = value;

Can we just use the original constants rather than defining these properties? Ths makes it hard to figure out where they come from.

::: toolkit/modules/addons/WebRequest.jsm:446
(Diff revision 2)
> +        let value = iface[c];
> +        this[c.replace(prefix, "ACTIVITY_")] = value;
> +        map.set(value, c.replace(prefix, "NS_ERROR_NET_ON_"));
> +      }
> +    }
> +    this.activityErrorsMap = map;

Can we have a lazy initializer for this map rather than for this entire method?

::: toolkit/modules/addons/WebRequest.jsm:454
(Diff revision 2)
> +  },
> +  // The real deal.
> +  _observeActivity(channel, activityType, activitySubtype /* , aTimestamp, aExtraSizeData, aExtraStringData */) {
> +    let channelData = getData(channel);
> +    let goodLastActivity = this.ACTIVITY_RESPONSE_HEADER;
> +    let lastActivity = channelData.lastActivity;

Please use something like `channelData.lastActivity || undefined` to avoid a warning.

::: toolkit/modules/addons/WebRequest.jsm:458
(Diff revision 2)
> +    let goodLastActivity = this.ACTIVITY_RESPONSE_HEADER;
> +    let lastActivity = channelData.lastActivity;
> +    if (activitySubtype === this.ACTIVITY_RESPONSE_COMPLETE && lastActivity && lastActivity !== goodLastActivity) {
> +      let loadContext = this.getLoadContext(channel);
> +      if (!this.errorCheck(channel, loadContext, channelData)) {
> +        this.runChannelListener(channel, loadContext, "onError", 

Please remove trailing whitespace.

::: toolkit/modules/addons/WebRequest.jsm:475
(Diff revision 2)
>    },
>  
> +  get errorsMap() {
> +    delete this.errorsMap;
> +    this.errorsMap = new Map(Object.keys(Cr)
> +                                   .filter(name => !Components.isSuccessCode(Cr[name]))

I don't think there's any reason to restrict this to error results. Maybe just rename it `resultNameMap` and include all result codes?

::: toolkit/modules/addons/WebRequest.jsm:481
(Diff revision 2)
> +                                   .map(name => [Cr[name], name]));
> +    return this.errorsMap;
> +  },
> +  maybeError(channel, extraData = null, channelData = null) {
> +    if (!(extraData && extraData.error)) {
> +      let channelStatus = channel.status;

I don't think this variable is helpful. Can we just use `channel.status` in the two lines below?

::: toolkit/modules/addons/WebRequest.jsm:508
(Diff revision 2)
> +        channelData.errorNotified = true;
> +      } else {
> +        if (this.errorCheck(channel, loadContext, channelData)) {
> -      return false;
> +          return false;
> +        } else {
> +          runLater(() => this.errorCheck(channel, loadContext, channelData));

Why?
Attachment #8731685 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 4

a year ago
https://reviewboard.mozilla.org/r/40775/#review38295

Thank you, I've been told so at the Monday meeting, hope you had a pleasant journey.

> Mochitests aren't allowed to make requests to external servers. Please choose one of the hosts listed in https://dxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt

I've read that document entirely, but I could not figure out which host I should pick to point an unresolved domain name which makes the request fail at the DNS resolution stage. Maybe I'm just too sleepy, could you point it out for me? Thank you :)

> `if (details.url.includes("_bad"))`

We've got a lot of indexOf(...) != -1 scattered all around this file. Probably whoever started writing the test was not aware yet of String.prototype.includes(). Should I fix all those 10 instances for consistency?

> Shouldn't this be `if (event !== "onRedirect" && e.result !== Cr.NS_ERROR_NOT_AVAILABLE)`?

I agree, assuming you actually mean `if (event !== "onRedirect" || e.result !== Cr.NS_ERROR_NOT_AVAILABLE)`.

> Can we just use the original constants rather than defining these properties? Ths makes it hard to figure out where they come from.

I'm not sure. If the JIT compiler is smart enough to figure out they're just constants and nit can skip the double XPConnect propery access dance every time we use them inside a very busy observer, definitely yes. Have you already got the answer or should I run some benchmarks?

> Can we have a lazy initializer for this map rather than for this entire method?

If we just need to initialize this map rather than the constants as well, yes. It depends on the answer to the previous question.

> Please use something like `channelData.lastActivity || undefined` to avoid a warning.

Fine with that, but which warning?

> I don't think there's any reason to restrict this to error results. Maybe just rename it `resultNameMap` and include all result codes?

We only use failure codes anyway, but since success code are just two, removing the filter is fine, and `resultsMap` will do I think.

> I don't think this variable is helpful. Can we just use `channel.status` in the two lines below?

You're right, since the second property access is the exceptional case.

> Why?

It was used to catch channels aborted by another synchronous on-open / on-modify HTTP observers, and if was needed before we added our nsIHttpActivityObserved because onStopRequest(), which was the place we previously checked for errors, never has a chance to run in that case.  But in this final version it is redundant, indeed.
https://reviewboard.mozilla.org/r/40775/#review38295

> I've read that document entirely, but I could not figure out which host I should pick to point an unresolved domain name which makes the request fail at the DNS resolution stage. Maybe I'm just too sleepy, could you point it out for me? Thank you :)

Hm. I'm not sure how to test that. External DNS resolution is still external network access. I'm not sure if the test harness would allow it, but we still shouldn't rely on it.

Maybe we have other tests that rely on NXDOMAIN results that you can look at?

> We've got a lot of indexOf(...) != -1 scattered all around this file. Probably whoever started writing the test was not aware yet of String.prototype.includes(). Should I fix all those 10 instances for consistency?

It would be nice.

> I agree, assuming you actually mean `if (event !== "onRedirect" || e.result !== Cr.NS_ERROR_NOT_AVAILABLE)`.

Right.

> I'm not sure. If the JIT compiler is smart enough to figure out they're just constants and nit can skip the double XPConnect propery access dance every time we use them inside a very busy observer, definitely yes. Have you already got the answer or should I run some benchmarks?

The JIT is not that smart, no, and XPConnect property access is quite slow. But these methods are not going to benefit much from the JIT, and, in any case, I'd rather try to avoid these kinds of micro-optimizations until the profiler suggests that we need them, rather than assume we do until it suggests otherwise.

> Fine with that, but which warning?

In general, accessing an undefined property in anything other than a boolean context generates a warning.
(Assignee)

Comment 6

a year ago
Comment on attachment 8731685 [details]
MozReview Request: Bug 1252596 - Implement webRequest.onErrorOccurred. r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40775/diff/2-3/
Attachment #8731685 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 7

a year ago
https://reviewboard.mozilla.org/r/40775/#review38295

> Hm. I'm not sure how to test that. External DNS resolution is still external network access. I'm not sure if the test harness would allow it, but we still shouldn't rely on it.
> 
> Maybe we have other tests that rely on NXDOMAIN results that you can look at?

OK, I'm gonna use invalid.localhost which works just fine as well.
BTW, thanks to our activity -> error mapping, the synthetic "no warranties" error description comes out quite nice: "NS_ERROR_NET_ON_RESOLVED".
Attachment #8731685 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8731685 [details]
MozReview Request: Bug 1252596 - Implement webRequest.onErrorOccurred. r?kmag

https://reviewboard.mozilla.org/r/40775/#review38583

Looks good. Thanks.

In the future, if you're only going to run your try jobs on one platform, please use linux32 or linux64. Windows is the most resource-constrained and the most heavily loaded platform, and these tests should not be especially platform-dependent.

::: toolkit/components/extensions/ext-webRequest.js:16
(Diff revision 3)
>  
>  Cu.import("resource://gre/modules/ExtensionUtils.jsm");
>  var {
>    SingletonEventManager,
>    runSafeSync,
>    ignoreEvent,

This is failing ESLint for being unused.
(Assignee)

Comment 9

a year ago
Comment on attachment 8731685 [details]
MozReview Request: Bug 1252596 - Implement webRequest.onErrorOccurred. r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40775/diff/3-4/
(Assignee)

Comment 10

a year ago
https://reviewboard.mozilla.org/r/40775/#review38583

I figured that much, and indeed so far I had always used linux32. However in Berlin I've been advised to launch Try on a platform different than the one where I develop and run my local tests, which actually makes sense too. But since, as you said, most of our WebExtensions code is indeed platform-independent, I'll go back to linux32 unless I've a strong reason to do otherwise.

> This is failing ESLint for being unused.

Fixed. Would you use MozReview's autoland or should I put the [checkin-needed] keyword (what's the norm these days?)
(Assignee)

Updated

a year ago
Keywords: checkin-needed
https://reviewboard.mozilla.org/r/40775/#review38583

> Fixed. Would you use MozReview's autoland or should I put the [checkin-needed] keyword (what's the norm these days?)

I don't think there is a norm. Either works.

Comment 12

a year ago
https://hg.mozilla.org/integration/fx-team/rev/2d81f8a108e1
Keywords: checkin-needed

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2d81f8a108e1
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.