1395322 - stylo: Linux 64 / Windows 7 / Windows 8 hang in StyleSheet::GetCssRules in layout/style/test/chrome/test_stylesheet_clone_import_rule.html

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Brad Werth [:bradwerth]]

Bug 1378577 resolved issues with incorrect waiting on Promises, but there was still a timeout after push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa2398342c71364ccac794d0e2a0928496a4d75c

Error log shows linux stylo runs that timeout in a call to CSSStyleSheet.cssRules.

Comment 1

[Brad Werth [:bradwerth]]

Attachment: Bug 1395322 Part 2: Change timeouts to failures in layout/style/test/chrome/test_stylesheet_clone_import_rule.html.

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

Comment 2

[Brad Werth [:bradwerth]]

Xidorn, can you please tell me how to trigger the Linux x64 Stylo debug mochitests as you did in the error log in comment 0? I've tried:

1) try syntax that includes linux64-stylo. I get linux64 instead.
2) manually adding the relevants jobs using "Add new jobs" in Treeherder. They don't start.
3) using mach try fuzzy. I don't get any tests remaining with the filter "linux stylo debug chrome !browser !devtools".

How did you do it?

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

For "mach try fuzzy", you need "mach try fuzzy --full" to see those tier-3 tasks. And on Treeherder, you need to check "tier 3" in "Tier" menu to see the result.

Comment 4

[Brad Werth [:bradwerth]]

Wonderful; thank you.

Comment 5

[Brad Werth [:bradwerth]]

Emilio, I can't replicate this on macOS. Are you willing to look on a Linux machine? I belieb Linux 64 Stylo debug is hanging at http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/layout/style/StyleSheet.cpp#394 during this test.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

I cannot understand how can it hang there :/

Comment 7

[Brad Werth [:bradwerth]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #6)
> I cannot understand how can it hang there :/

It's weird, but it's indicated by the log: https://treeherder.mozilla.org/logviewer.html#?job_id=127184675&repo=try&lineNumber=8364

Comment 8

[Brad Werth [:bradwerth]]

Considering EnsureSafeToHandoutCSSRules and all that it does (EnsureUniqueInner, which could clone, etc), there's a lot that could go wrong when trying to retrive the "cssRules" property.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

I could reproduce it once, and we were just waiting polling on the main thread... I'll try to get an rr trace.

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Windows 7 / 8 opt / pgo runs are also hitting this in my enable Stylo try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c2998f33d9a493d9e5350a63da832d61333f24b&filter-searchStr=M(c3)

I filed bug 1395751 to skip the test for those runs until we can fix it.

Comment 11

[Intermittent Failures Robot]

7 failures in 939 pushes (0.007 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 7

Platform breakdown:
* linux64-stylo: 5
* linux32-stylo: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1395322&startday=2017-08-28&endday=2017-09-03&tree=all

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Brad Werth [:bradwerth] from comment #8)
> Considering EnsureSafeToHandoutCSSRules and all that it does
> (EnsureUniqueInner, which could clone, etc), there's a lot that could go
> wrong when trying to retrive the "cssRules" property.

There is indeed a lot that could go wrong... But if we hang inside native code, how can we recover from that? How can other js code continue running at all given this is a non-e10s test?

If it hangs and eventually killed by the script, then we may be able to know what's hanging from a backtrace. But in this case, we just don't hand the control back to the original js code, which sounds rather weird.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

I reproduced once on Windows... and fail to reproduce it anymore :/

That time, I attached the debugger on the process, and it seems we've returned to the main loop...

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

jryans, could you try this test on asan and see if there is any special alert?

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

This is probably one of the very few tests we are training the stylesheet cloning code I suppose. So bumping to P2.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Based on several experiment, I think there are two problems which cause this to timeout. One is in the test itself, the other is the problem in style system.

The test problem is that, it uses an async function, and if there is any exception thrown inside the async function, the test would timeout without any message. We need to explicitly catch the exception on the Promise object.

The style system problem is that importSheet2 is somehow incomplete, and thus it throws an exception when we try to get cssRules from it. This would be the real problem we need to investigate.

Despite that, it seems to me that if I add the needed ".catch()" and ".then()", the failure doesn't happen anymore... which is weird...

I'll see if we can enable the test with just this test change.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

OK, so it doesn't fix. The intermittent still exists, but it now shows useful message rather than just timeout: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93386a72ab9fec8bc07d3f43bbd8c1ac4dec88ab&selectedJob=128439985

Comment 18

[Brad Werth [:bradwerth]]

Comment on attachment 8902891 [details]
Bug 1395322 Part 2: Change timeouts to failures in layout/style/test/chrome/test_stylesheet_clone_import_rule.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174608/diff/1-2/

Comment 19

[Brad Werth [:bradwerth]]

Attachment 8902891 [details] is a patch of the test that turns timeouts into failures. I still can't reproduce on macOS.

Comment 20

[Brad Werth [:bradwerth]]

Found this probably-relevant comment https://bugzilla.mozilla.org/show_bug.cgi?id=761236#c1 from bz on Bug 761236:

> You get NS_ERROR_DOM_INVALID_ACCESS_ERR if you try to read .cssRules on a stylesheet that's not done being parsed yet.

I'll see if I can get further on this without being able to reproduce it.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

FWIW, this is a try run which shows that we actually get the StyleRuleAdded event for both import rules: https://treeherder.mozilla.org/#/jobs?repo=try&revision=297bda55a7d1092c9bb99a991c09c43595e03df8

So it would be weird that one of the rule being incomplete.

Comment 22

[Brad Werth [:bradwerth]]

Comment on attachment 8902891 [details]
Bug 1395322 Part 2: Change timeouts to failures in layout/style/test/chrome/test_stylesheet_clone_import_rule.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174608/diff/2-3/

Comment 23

[Brad Werth [:bradwerth]]

I have now seen the bug reproduce on my macOS build intermittently. The times I've been able to catch it in the debugger, it shows that sheet->Inner()->mComplete is false for the first import sheet, but I've not been able to determine when that change happens. It's notable that this failure is happening before any modifications have happened to the two imported sheets, so no stylesheet cloning has been triggered. In my local build, I added an assert to ServoStyleSheet::StyleSheetLoaded to ensure that mComplete is true at the point that the import rule StyleRuleAdded event is fired. That assert is not triggered during the few runs I've had with test failure.

I had a theory that the rule object which is sent out with the StyleRuleAdded event (where mComplete is true) is not the same rule object we get from checking the cssRules property of the primary stylesheet. I rebuilt the test to check it, but that part passes even when I get the intermittent failure on retrieving the cssRules property of the first imported sheet. Something else...

Comment 24

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e95c5268d096a27a510f91e20a49e25f10d6f8d4

Comment 25

[Brad Werth [:bradwerth]]

I've figured out the issue, though not yet the fix. Servo_CssRules_InsertRule is returning a rule type value that is inconsistent with the gecko constants at http://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/layout/style/Rule.h#68. Because of that, ServoStyleSheet::InsertRuleInternal incorrectly sends the StyleRuleAdded event for the import rule before the sheet has been loaded.

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

Assign back to Brad since it seems he's actively investigating.

Comment 27

[Brad Werth [:bradwerth]]

Attachment: Bug 1395322 Part 1: Enforce consistent use of nsIDOMCSSRule enums for Servo rule types.

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

Comment 28

[Brad Werth [:bradwerth]]

Attachment: Bug 1395322 Part 2: Make ServoCSSRuleList::InsertRule map DOMCSSRule types to Rule types.

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

Comment 29

[Brad Werth [:bradwerth]]

Comment on attachment 8902891 [details]
Bug 1395322 Part 2: Change timeouts to failures in layout/style/test/chrome/test_stylesheet_clone_import_rule.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174608/diff/3-4/

Comment 30

[Brad Werth [:bradwerth]]

Comment on attachment 8905297 [details]
Bug 1395322 Part 1: Enforce consistent use of nsIDOMCSSRule enums for Servo rule types.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177092/diff/1-2/

Comment 31

[Brad Werth [:bradwerth]]

Comment on attachment 8902891 [details]
Bug 1395322 Part 2: Change timeouts to failures in layout/style/test/chrome/test_stylesheet_clone_import_rule.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174608/diff/4-5/

Comment 32

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #25)
> Servo_CssRules_InsertRule is returning a rule type value that is
> inconsistent with the gecko constants

The issue is that ServoCSSRuleList uses nsIDOMCSSRule enums for rule types, but ServoStyleSheet::InsertRuleInternal was using gecko Rule enums. The patch makes ServoStyleSheet use the correct enums. The patch also renames the GetRuleType function to make clear what domain the rule type enums are coming from.

Comment 33

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8905297 [details]
Bug 1395322 Part 1: Enforce consistent use of nsIDOMCSSRule enums for Servo rule types.

https://reviewboard.mozilla.org/r/177092/#review182098

Ah... Good catch.

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8902891 [details]
Bug 1395322 Part 2: Change timeouts to failures in layout/style/test/chrome/test_stylesheet_clone_import_rule.html.

https://reviewboard.mozilla.org/r/174608/#review182104

::: layout/style/test/chrome/test_stylesheet_clone_import_rule.html:66
(Diff revision 5)
>  
> -    // Get the imported sheets and confirm they are non-null and have rules.
> -    let importSheet1 = stylesheet.cssRules[0].styleSheet;
> -    let importSheet2 = stylesheet.cssRules[1].styleSheet;
> -
> -    ok(importSheet1 && importSheet2, "Imported sheets exist.");
> +    // Do some sanity checking of our import rules.
> +    let primaryRules = stylesheet.cssRules;
> +    for (let i = 0; i < 2; ++i) {
> +      // Check that the main stylesheet rule is the same rule we saw in the event listener.
> +      is(primaryRules[i], events[i].rule, "Rule " + i + " matches the event listener.");

You can always use JavaScript's template literals to simplify the messages: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22af8e4748e3
Part 1: Enforce consistent use of nsIDOMCSSRule enums for Servo rule types. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/865d5faef4a7
Part 2: Change timeouts to failures in layout/style/test/chrome/test_stylesheet_clone_import_rule.html. r=xidorn

Comment 36

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/22af8e4748e3
https://hg.mozilla.org/mozilla-central/rev/865d5faef4a7

Comment 37

[Emilio Cobos Álvarez [:emilio]]

This didn't re-enable the test on stylo, was that intentional?

Comment 38

[Emilio Cobos Álvarez [:emilio]]

https://searchfox.org/mozilla-central/rev/b469db5c90d618f4b202d5ef280e1a78224eb43b/layout/style/test/chrome/chrome.ini#28

Comment 39

[Brad Werth [:bradwerth]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #37)
> This didn't re-enable the test on stylo, was that intentional?

That is an oversight. I've opened Bug 1444227 to re-enable the test.