Closed Bug 1455405 Opened 2 years ago Closed Last year

Intermittent test_ext_webrequest_basic.html | Good CSS loaded - got "rgb(0, 0, 0)", expected "rgb(255, 0, 0)"

Categories

(WebExtensions :: General, defect)

defect
Not set

Tracking

(firefox62 fixed, firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: mixedpuppy)

References

Details

(Keywords: intermittent-failure, Whiteboard: [retriggered][stockwell fixed:other])

Attachments

(2 files, 1 obsolete file)

Filed by: csabou [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=174598419&repo=autoland

https://queue.taskcluster.net/v1/task/K88hHcmpQIC3NsK4OWn0xA/runs/0/artifacts/public/logs/live_backing.log

[task 2018-04-19T18:34:49.658Z] 18:34:49     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | Good CSS loaded - got "rgb(0, 0, 0)", expected "rgb(255, 0, 0)"
[task 2018-04-19T18:34:49.658Z] 18:34:49     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:312:5
[task 2018-04-19T18:34:49.659Z] 18:34:49     INFO -     test_webRequest_links@toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html:82:3
[task 2018-04-19T18:34:49.660Z] 18:34:49     INFO -     async*add_task/</<@SimpleTest/SpawnTask.js:298:21
[task 2018-04-19T18:34:49.661Z] 18:34:49     INFO -     onFulfilled@SimpleTest/SpawnTask.js:69:15
[task 2018-04-19T18:34:49.662Z] 18:34:49     INFO -     promise callback*next@SimpleTest/SpawnTask.js:104:45
[task 2018-04-19T18:34:49.662Z] 18:34:49     INFO -     onFulfilled@SimpleTest/SpawnTask.js:73:7
[task 2018-04-19T18:34:49.663Z] 18:34:49     INFO -     co/<@SimpleTest/SpawnTask.js:58:5
[task 2018-04-19T18:34:49.663Z] 18:34:49     INFO -     co@SimpleTest/SpawnTask.js:54:10
[task 2018-04-19T18:34:49.664Z] 18:34:49     INFO -     add_task/<@SimpleTest/SpawnTask.js:272:9
[task 2018-04-19T18:34:49.664Z] 18:34:49     INFO -     setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:676:12
[task 2018-04-19T18:34:49.665Z] 18:34:49     INFO -     add_task@SimpleTest/SpawnTask.js:271:7
[task 2018-04-19T18:34:49.665Z] 18:34:49     INFO -     @toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html:28:1
[task 2018-04-19T18:34:49.666Z] 18:34:49     INFO - SpawnTask.js | Leaving test test_webRequest_links
[task 2018-04-19T18:34:49.667Z] 18:34:49     INFO - SpawnTask.js | Entering test test_webRequest_images
[task 2018-04-19T18:34:49.667Z] 18:34:49     INFO - onBeforeRequest 18 http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test-oop-extensions/file_image_bad.png
Duplicate of this bug: 1456360
I look into this.
Assignee: nobody → mixedpuppy
that went well.
Attached patch disablelinuxx64_bug1455405 (obsolete) — Splinter Review
Attachment #8971956 - Flags: review?(jmaher)
Flags: needinfo?(gbrown)
Comment on attachment 8971956 [details] [diff] [review]
disablelinuxx64_bug1455405

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

this is r+ on the first try!
Attachment #8971956 - Flags: review?(jmaher) → review+
Whiteboard: [retriggered] [stockwell disable-recommended] → [retriggered][stockwell disabled]
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/496872fda350
disable test_ext_webrequest_basic.html on Linux x64 on all build types except asan r=jmaher
Keywords: checkin-needed
This bug is still failing on Linux mostly, affecting debug, opt. It has failed 36 times in the last 7 days.
Recent log:https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=177070141&lineNumber=26171

:jmaher,could you please take a look? Should we disable it on Linux too?
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(jmaher)
Resolution: --- → INVALID
This is still happening.

Recent log: https://treeherder.mozilla.org/logviewer.html#?job_id=178013760&repo=autoland&lineNumber=19630

[task 2018-05-11T08:51:14.265Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | received onResponseStarted 
[task 2018-05-11T08:51:14.266Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | resource type is correct - Expected: stylesheet, Actual: stylesheet 
[task 2018-05-11T08:51:14.268Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | origin is correct - Expected: http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html, Actual: http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html 
[task 2018-05-11T08:51:14.269Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | origin is correct - Expected: http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html, Actual: http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html 
[task 2018-05-11T08:51:14.270Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | correct requestId - Expected: 16, Actual: 16 
[task 2018-05-11T08:51:14.270Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | correct tabId - Expected: 2, Actual: 2 
[task 2018-05-11T08:51:14.271Z] 08:51:14     INFO - onCompleted 16 http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test-oop-extensions/file_style_good.css
[task 2018-05-11T08:51:14.273Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | received onCompleted 
[task 2018-05-11T08:51:14.274Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | resource type is correct - Expected: stylesheet, Actual: stylesheet 
[task 2018-05-11T08:51:14.275Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | origin is correct - Expected: http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html, Actual: http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html 
[task 2018-05-11T08:51:14.276Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | origin is correct - Expected: http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html, Actual: http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html 
[task 2018-05-11T08:51:14.277Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | correct requestId - Expected: 16, Actual: 16 
[task 2018-05-11T08:51:14.278Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | correct tabId - Expected: 2, Actual: 2 
[task 2018-05-11T08:51:14.280Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | IP for http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test-oop-extensions/file_style_good.css looks IP-ish: 127.0.0.1 
[task 2018-05-11T08:51:14.281Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | correct ip for http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test-oop-extensions/file_style_good.css - Expected: 127.0.0.1, Actual: 127.0.0.1 
[task 2018-05-11T08:51:14.282Z] 08:51:14     INFO - Buffered messages finished
[task 2018-05-11T08:51:14.283Z] 08:51:14     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | Good CSS loaded - got "rgb(0, 0, 0)", expected "rgb(255, 0, 0)"
[task 2018-05-11T08:51:14.284Z] 08:51:14     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:312:5
[task 2018-05-11T08:51:14.285Z] 08:51:14     INFO -     test_webRequest_links@toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html:82:3
[task 2018-05-11T08:51:14.286Z] 08:51:14     INFO -     async*add_task/</<@SimpleTest/AddTask.js:57:34
[task 2018-05-11T08:51:14.287Z] 08:51:14     INFO -     async*add_task/<@SimpleTest/AddTask.js:31:10
[task 2018-05-11T08:51:14.288Z] 08:51:14     INFO -     setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:676:12
[task 2018-05-11T08:51:14.289Z] 08:51:14     INFO -     add_task@SimpleTest/AddTask.js:30:7
[task 2018-05-11T08:51:14.290Z] 08:51:14     INFO -     @toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html:28:1
[task 2018-05-11T08:51:14.291Z] 08:51:14     INFO - AddTask.js | Leaving test test_webRequest_links
[task 2018-05-11T08:51:14.292Z] 08:51:14     INFO - AddTask.js | Entering test test_webRequest_images
[task 2018-05-11T08:51:14.293Z] 08:51:14     INFO - onBeforeRequest 17 http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test-oop-extensions/file_image_bad.png
[task 2018-05-11T08:51:14.294Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | received onBeforeRequest 
[task 2018-05-11T08:51:14.295Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | resource type is correct - Expected: image, Actual: image 
[task 2018-05-11T08:51:14.297Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | origin is correct - Expected: http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html, Actual: http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html 
[task 2018-05-11T08:51:14.298Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | origin is correct - Expected: http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html, Actual: http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html 
[task 2018-05-11T08:51:14.299Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | tabId 2 
[task 2018-05-11T08:51:14.300Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | requestId 17 
[task 2018-05-11T08:51:14.301Z] 08:51:14     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | valid resource type image 
[task 2018-05-11T08:51:14.302Z] 08:51:14     INFO - onBeforeRequest cancel request
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
it looks like we need to disable this on linux32 as well, Bogdan, do you want to adjust the previous patch to remove the bits=='64' ?
Flags: needinfo?(jmaher) → needinfo?(btara)
:jmaher Is the patch adequate? :)
Attachment #8971956 - Attachment is obsolete: true
Flags: needinfo?(btara)
Attachment #8975053 - Flags: review?(jmaher)
Comment on attachment 8975053 [details] [diff] [review]
disable test_ext_webrequest_basic.html on linux 32 and 64 for frequent failures

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

the only change here is I don't see your username in the patch, if you want to upload again, we can do that or land this and whoever lands it will be the author
Attachment #8975053 - Flags: review?(jmaher) → review+
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9918e11ae725
disable test_ext_webrequest_basic.html on linux 32 and 64 for frequent failures. r=jmaher
Keywords: checkin-needed
Product: Toolkit → WebExtensions
Summary: Intermittent toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | Good CSS loaded - got "rgb(0, 0, 0)", expected "rgb(255, 0, 0)" → Intermittent test_ext_webrequest_basic.html | Good CSS loaded - got "rgb(0, 0, 0)", expected "rgb(255, 0, 0)"
Duplicate of this bug: 1467099
Comment on attachment 8990026 [details]
Bug 1455405 fix intermittent by using real events,

https://reviewboard.mozilla.org/r/255058/#review261880


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html:18
(Diff revision 1)
>  "use strict";
>  
> -function spinEventLoop() {
> -  return new Promise((resolve, reject) => {
> -    window.requestIdleCallback(() => {
> -      resolve();
> +function promiseWindowEvent(name, accept) {
> +  return new Promise(resolve => {
> +    window.addEventListener(name, function listener(event) {
> +      console.log("message received", event);

Error: Unexpected console statement. [eslint: no-console]
Comment on attachment 8990026 [details]
Bug 1455405 fix intermittent by using real events,

https://reviewboard.mozilla.org/r/255058/#review261904

Looks good, follows some additonal notes/comments.

They are related to a small nit, and some notes about an addional strategy (`ContentTaskUtils.waitForCondition`) that we may use to still assert that the CSS stylesheet has been applied, and one about the assertion related to JS files with `cached === true`, in case we want to keep those assertion in this test.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html
(Diff revision 2)
> -  await spinEventLoop();
> -  let style = window.getComputedStyle(document.getElementById("test"));
> -  is(style.getPropertyValue("color"), "rgb(255, 0, 0)", "Good CSS loaded");

It sounds good to me to remove the explicit assertion on the stylesheet rules that we expect to be applied, if we are already verify that in other tests and we don't have a reliable way to do verify this part of the expected behaviors in this test case.

But I'm wondering if using `ContentTaskUtils.waitForCondition` could be a more reliable way than the previous `spinEventLoop` hack that has being used in this test.

Something like:

```
<script>
"use strict";

var {ContentTaskUtils} = SpecialPowers.Cu.import("resource://testing-common/ContentTaskUtils.jsm", {});

...

// we redirect to style_good which completes the test
  addStylesheet("file_style_redirect.css");
  await extension.awaitMessage("done");

  // waiting for CSS applied.
  info('Wait the expected stylesheet rules to be applied');
  await ContentTaskUtils.waitForCondition(() => {
    let style = window.getComputedStyle(document.getElementById("test"));
    return style.getPropertyValue("color") === "rgb(255, 0, 0)";
  });
});
```

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html:141
(Diff revision 2)
>    await extension.awaitMessage("cancelled");
>    // we redirect to script_good which completes the test
> -  addScript("file_script_redirect.js");
> +  addScript("file_script_redirect.js?q=test1");
>    await extension.awaitMessage("done");
>  
> -  // wait for JS executed
> +  is((await message).data, "test1", "good script ran");

Nit, if we are never interested to check the other properties of the postMessage event, we may change `promiseWindowEvent` to return a promise which resolves to `event.data` and then change this assertion into `is(await message, "test1", "...")`.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html:181
(Diff revision 2)
>        type: "image",
>        cached: true,
>      },
>      "file_script_good.js": {
>        type: "script",
> -      cached: true,
> +      cached: false,

It seems that we had to change this from `cached: true` to `cached: false` because we are loading "file_script_good.js?q=test1" in a previous test case and "file_script_good.js?q=test2" in this test.

We could still test that the js files gets the `cached` property set to `true` by either use "file_script_good.js?q=test1" in this test as well.

If you think that test that `cached === true` for the js files, we could at least add a comment that explains why this is expected to not be cached (because it is not going to be immediately clear by  quickly looking at the test)

(I guess that another option would also be to use `ContentTaskUtils.waitForCondition` instead of the `spinEventLoop` hack to test that the expected js files are loaded (or not loaded if they are not supposed to be loaded) doing something similar to the change suggested for the CSS part:

```
info(`Wait for the JS files to be executed`);
await ContentTaskUtils.waitForCondition(() => {
   return window.success === 2 && window.failure === failure;
});
```
)
Attachment #8990026 - Flags: review?(lgreco) → review+
Comment on attachment 8990026 [details]
Bug 1455405 fix intermittent by using real events,

https://reviewboard.mozilla.org/r/255058/#review261904

> It sounds good to me to remove the explicit assertion on the stylesheet rules that we expect to be applied, if we are already verify that in other tests and we don't have a reliable way to do verify this part of the expected behaviors in this test case.
> 
> But I'm wondering if using `ContentTaskUtils.waitForCondition` could be a more reliable way than the previous `spinEventLoop` hack that has being used in this test.
> 
> Something like:
> 
> ```
> <script>
> "use strict";
> 
> var {ContentTaskUtils} = SpecialPowers.Cu.import("resource://testing-common/ContentTaskUtils.jsm", {});
> 
> ...
> 
> // we redirect to style_good which completes the test
>   addStylesheet("file_style_redirect.css");
>   await extension.awaitMessage("done");
> 
>   // waiting for CSS applied.
>   info('Wait the expected stylesheet rules to be applied');
>   await ContentTaskUtils.waitForCondition(() => {
>     let style = window.getComputedStyle(document.getElementById("test"));
>     return style.getPropertyValue("color") === "rgb(255, 0, 0)";
>   });
> });
> ```

I would rather not even deal with a test like this here.  The underlying platform dom/css tests should already deal with ensuring css works as expected.  This test is just kinda there "because".

> It seems that we had to change this from `cached: true` to `cached: false` because we are loading "file_script_good.js?q=test1" in a previous test case and "file_script_good.js?q=test2" in this test.
> 
> We could still test that the js files gets the `cached` property set to `true` by either use "file_script_good.js?q=test1" in this test as well.
> 
> If you think that test that `cached === true` for the js files, we could at least add a comment that explains why this is expected to not be cached (because it is not going to be immediately clear by  quickly looking at the test)
> 
> (I guess that another option would also be to use `ContentTaskUtils.waitForCondition` instead of the `spinEventLoop` hack to test that the expected js files are loaded (or not loaded if they are not supposed to be loaded) doing something similar to the change suggested for the CSS part:
> 
> ```
> info(`Wait for the JS files to be executed`);
> await ContentTaskUtils.waitForCondition(() => {
>    return window.success === 2 && window.failure === failure;
> });
> ```
> )

This was based on the previous revision.
Comment on attachment 8990026 [details]
Bug 1455405 fix intermittent by using real events,

https://reviewboard.mozilla.org/r/255058/#review261904

> Nit, if we are never interested to check the other properties of the postMessage event, we may change `promiseWindowEvent` to return a promise which resolves to `event.data` and then change this assertion into `is(await message, "test1", "...")`.

This is just how postmessage works.
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/21074760bff6
fix intermittent by using real events, r=rpl
Status: REOPENED → RESOLVED
Closed: 2 years agoLast year
Keywords: leave-open
Resolution: --- → FIXED
Whiteboard: [retriggered][stockwell disabled] → [retriggered][stockwell fixed]
Target Milestone: --- → mozilla63
Whiteboard: [retriggered][stockwell fixed] → [retriggered][stockwell fixed:other]
You need to log in before you can comment on or make changes to this bug.