Closed Bug 1252596 Opened 8 years ago Closed 8 years ago

Implement webRequest.onErrorOccurred

Categories

(WebExtensions :: Untriaged, defect)

47 Branch
x86
macOS
defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
48.1 - Mar 21
Tracking Status
firefox48 --- fixed

People

(Reporter: josesigna, Assigned: ma1)

References

()

Details

(Whiteboard: [webRequest] triaged)

Attachments

(1 file)

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/
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86
Flags: blocking-webextensions+
Assignee: nobody → g.maone
Flags: blocking-webextensions+ → blocking-webextensions?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Status: NEW → ASSIGNED
Summary: webRequest.onErrorOccurred listed as implemented but it's not → Implement webRequest.onErrorOccurred
Blocks: 1235639
Iteration: --- → 48.1 - Mar 21
Flags: blocking-webextensions? → blocking-webextensions+
Whiteboard: [webRequest] triaged
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)
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.
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)
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.
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/
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?)
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.
https://hg.mozilla.org/mozilla-central/rev/2d81f8a108e1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.