Closed Bug 1429744 Opened 2 years ago Closed 2 years ago

Perma failing layout/inspector/tests/chrome/test_parseStyleSheetObservers.html | Test 2: Got the style rule. - got false, expected true

Categories

(Core :: Layout, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox59 --- fixed
firefox60 --- fixed

People

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

Details

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

Attachments

(1 file)

Filed by: apavel [at] mozilla.com

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

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

[task 2018-01-11T10:54:10.072Z] 10:54:10     INFO - TEST-START | layout/inspector/tests/chrome/test_parseStyleSheetObservers.html
[task 2018-01-11T10:54:10.121Z] 10:54:10     INFO - GECKO(1149) | ++DOMWINDOW == 18 (0x7f74a41ec800) [pid = 1149] [serial = 18] [outer = 0x7f74b0985e20]
[task 2018-01-11T10:54:10.245Z] 10:54:10     INFO - TEST-INFO | started process screentopng
[task 2018-01-11T10:54:10.694Z] 10:54:10     INFO - TEST-INFO | screentopng: exit 0
[task 2018-01-11T10:54:10.696Z] 10:54:10     INFO - Buffered messages logged at 10:54:10
[task 2018-01-11T10:54:10.697Z] 10:54:10     INFO - TEST-PASS | layout/inspector/tests/chrome/test_parseStyleSheetObservers.html | Test 1: Stylesheet now has 2 rules. 
[task 2018-01-11T10:54:10.698Z] 10:54:10     INFO - TEST-PASS | layout/inspector/tests/chrome/test_parseStyleSheetObservers.html | Test 1: Removed expected number of rules. 
[task 2018-01-11T10:54:10.700Z] 10:54:10     INFO - TEST-PASS | layout/inspector/tests/chrome/test_parseStyleSheetObservers.html | Test 1: Added expected number of rules. 
[task 2018-01-11T10:54:10.702Z] 10:54:10     INFO - test2Setup: called
[task 2018-01-11T10:54:10.703Z] 10:54:10     INFO - TEST-PASS | layout/inspector/tests/chrome/test_parseStyleSheetObservers.html | Test 2: Stylesheet now has 2 rules. 
[task 2018-01-11T10:54:10.705Z] 10:54:10     INFO - styleFirstAddProcessor: called with event @import url("imported_no_op.css");
[task 2018-01-11T10:54:10.705Z] 10:54:10     INFO - Buffered messages finished
[task 2018-01-11T10:54:10.706Z] 10:54:10     INFO - TEST-UNEXPECTED-FAIL | layout/inspector/tests/chrome/test_parseStyleSheetObservers.html | Test 2: Got the style rule. - got false, expected true
[task 2018-01-11T10:54:10.707Z] 10:54:10     INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:312:5
[task 2018-01-11T10:54:10.707Z] 10:54:10     INFO - test2Setup@chrome://mochitests/content/chrome/layout/inspector/tests/chrome/test_parseStyleSheetObservers.html:92:3
[task 2018-01-11T10:54:10.709Z] 10:54:10     INFO - TEST-PASS | layout/inspector/tests/chrome/test_parseStyleSheetObservers.html | Test 2: Got the import rule. 
[task 2018-01-11T10:54:10.710Z] 10:54:10     INFO - GECKO(1149) | MEMORY STAT | vsize 2012MB | residentFast 285MB | heapAllocated 92MB
[task 2018-01-11T10:54:10.715Z] 10:54:10     INFO - TEST-OK | layout/inspector/tests/chrome/test_parseStyleSheetObservers.html | took 227ms
[task 2018-01-11T10:54:10.717Z] 10:54:10     INFO - GECKO(1149) | ++DOMWINDOW == 19 (0x7f74a6ab0400) [pid = 1149] [serial = 19] [outer = 0x7f74b0985e20]
[task 2018-01-11T10:54:10.718Z] 10:54:10     INFO - TEST-START | Shutdown
[task 2018-01-11T10:54:10.719Z] 10:54:10     INFO - Passed:  12
[task 2018-01-11T10:54:10.720Z] 10:54:10     INFO - Failed:  1
[task 2018-01-11T10:54:10.721Z] 10:54:10     INFO - Todo:    0
[task 2018-01-11T10:54:10.722Z] 10:54:10     INFO - Mode:    non-e10s
[task 2018-01-11T10:54:10.724Z] 10:54:10     INFO - Slowest: 1148ms - chrome://mochitests/content/chrome/layout/inspector/tests/chrome/test_parseStyleSheetObservers.html
[task 2018-01-11T10:54:10.725Z] 10:54:10     INFO - TEST-INFO | Ran 2 Loops
[task 2018-01-11T10:54:10.726Z] 10:54:10     INFO - SimpleTest FINISHED
In the last 7 days this bug has failed 30 times. The fails happen on Linux x64, MacOSX 64 nightly, OSX 10.10, Windows 7, windows 10-64 and windows 7-32 nightly

Here is an example of the most recent log: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-central&job_id=155585821&lineNumber=1624

Here is a relevant snippet from aforementioned log:

02:38:16     INFO - TEST-UNEXPECTED-FAIL | layout/inspector/tests/chrome/test_parseStyleSheetObservers.html | Test 2: Got the style rule. - got false, expected true
02:38:16     INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:312:5
02:38:16     INFO - test2Setup@chrome://mochitests/content/chrome/layout/inspector/tests/chrome/test_parseStyleSheetObservers.html:92:3
02:38:16     INFO - TEST-PASS | layout/inspector/tests/chrome/test_parseStyleSheetObservers.html | Test 2: Got the import rule. 
02:38:16     INFO - GECKO(753) | MEMORY STAT | vsize 4358MB | residentFast 339MB | heapAllocated 97MB
02:38:16     INFO - TEST-OK | layout/inspector/tests/chrome/test_parseStyleSheetObservers.html | took 131ms
02:38:16     INFO - GECKO(753) | ++DOMWINDOW == 18 (0x1147b2400) [pid = 753] [serial = 18] [outer = 0x1217e8e10]
02:38:16     INFO - TEST-START | Shutdown
Flags: needinfo?(bugs)
Whiteboard: [stockwell needswork]
Brad: the test failures reported above don't reflect the comment here: 

  // Create a Promise to watch for two StyleRuleAdded events. The first invocation should
  // be the style rule, and the second should be the import rule. We use the same processor
  // for both events, but the processor will only return true (completing the Promise) when
  // the import rule has been processed.

from:
https://searchfox.org/mozilla-central/source/layout/inspector/tests/chrome/test_parseStyleSheetObservers.html

Can you double-check the assertions here? Thx!
Flags: needinfo?(bugs) → needinfo?(bwerth)
This failure is happening because the import rule StyleRuleAdded event is arriving before the style rule StyleRuleAdded event. That's not a test problem; it's a code problem and I'll have to dig a bit. I'll also improve the test so that the failure message in this case states the true cause of the failure.
Huh, well I can't actually find the bit of code that enforces import rules to be deferred after style rules. The deferral is mediated through a check to StyleSheet::RuleHasPendingChildSheet -- and if the rule has the sheet ready, then the processing of the rule isn't deferred at all. This seems correct. So there's an assumption in the test that isn't enforced by the code.

I'm updating the test to reflect this new understanding. It makes the test less meaningful, but it won't fail for this reason any more.
Flags: needinfo?(bwerth)
Assignee: nobody → bwerth
Attachment #8942279 - Flags: review?(cam)
Attachment #8942279 - Flags: review?(cam) → review?(emilio)
Comment on attachment 8942279 [details]
Bug 1429744: Relax a test of StyleRuleAdded events to allow for the possibility that style rules could arrive before import rules.

https://reviewboard.mozilla.org/r/212566/#review220108

Yeah, this test is racy. The reason the order may change is because of the `RuleHasPendingChildSheet` check here:

  https://searchfox.org/mozilla-central/rev/9f2721b19b10a9a441fa5ca7116720d2fcebcc4a/layout/style/ServoStyleSheet.cpp#323

I think we may want to kill that weird notification order, but maybe the inspector relies on import rules being added already having its associated sheet? We could file a bug to investigate it.

Anyway, there's nothing preventing that sheet to be in the cache and be loaded sync.

Could you double-check / confirm that we actually have a stylesheet there, and that it's loaded?

Also, pointing this out in the commit message would be great.

r=me with that, thanks for fixing this!
Attachment #8942279 - Flags: review?(emilio) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/922a011fced2
Relax a test of StyleRuleAdded events to allow for the possibility that style rules could arrive before import rules. r=emilio
Comment on attachment 8942279 [details]
Bug 1429744: Relax a test of StyleRuleAdded events to allow for the possibility that style rules could arrive before import rules.

https://reviewboard.mozilla.org/r/212566/#review220322


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

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


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


::: layout/inspector/tests/chrome/test_parseStyleSheetObservers.html:68
(Diff revision 2)
>  let foundStyle = false;
>  let styleFirstAddProcessor = function(event) {
>    info("styleFirstAddProcessor: called with event "+ event.rule.cssText);
>    if (event.rule.type == CSSRule.IMPORT_RULE) {
>      foundImport = true;
> +    isnot(event.rule.styleSheet, null, "Test 2: import rule has stylesheet loaded.");

Error: 'isnot' is not defined. [eslint: no-undef]
(In reply to Code Review Bot [:reviewbot] from comment #11)
> ::: layout/inspector/tests/chrome/test_parseStyleSheetObservers.html:68
> (Diff revision 2)
> >  let foundStyle = false;
> >  let styleFirstAddProcessor = function(event) {
> >    info("styleFirstAddProcessor: called with event "+ event.rule.cssText);
> >    if (event.rule.type == CSSRule.IMPORT_RULE) {
> >      foundImport = true;
> > +    isnot(event.rule.styleSheet, null, "Test 2: import rule has stylesheet loaded.");
> 
> Error: 'isnot' is not defined. [eslint: no-undef]

This test should be on the ignore list; not sure why lint triggered on it. Running lint locally shows that all the SimpleTest usages fail. Setting "checkin-needed" to get help on landing it.
Keywords: checkin-needed
We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.
https://hg.mozilla.org/mozilla-central/rev/922a011fced2
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Whiteboard: [stockwell needswork] → [stockwell fixed:logic]
You need to log in before you can comment on or make changes to this bug.