Closed
Bug 1395322
Opened 7 years ago
Closed 7 years ago
stylo: Linux 64 / Windows 7 / Windows 8 hang in StyleSheet::GetCssRules in layout/style/test/chrome/test_stylesheet_clone_import_rule.html
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: bradwerth, Assigned: bradwerth)
References
Details
(Keywords: intermittent-failure)
Attachments
(2 files, 1 obsolete file)
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.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bwerth
Comment hidden (mozreview-request) |
Blocks: stylo-intermittent-test-failures
Keywords: intermittent-failure
Assignee | ||
Comment 2•7 years ago
|
||
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?
Flags: needinfo?(xidorn+moz)
Comment 3•7 years ago
|
||
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.
Flags: needinfo?(xidorn+moz)
Assignee | ||
Comment 4•7 years ago
|
||
Wonderful; thank you.
Assignee | ||
Comment 5•7 years ago
|
||
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.
Flags: needinfo?(emilio)
Summary: Possible timeouts remaining in layout/style/test/chrome/test_stylesheet_clone_import_rule.html → stylo: Linux 64 hang in StyleSheet::GetCssRules in layout/style/test/chrome/test_stylesheet_clone_import_rule.html
Comment 6•7 years ago
|
||
I cannot understand how can it hang there :/
Assignee | ||
Comment 7•7 years ago
|
||
(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
Assignee | ||
Comment 8•7 years ago
|
||
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•7 years ago
|
||
I could reproduce it once, and we were just waiting polling on the main thread... I'll try to get an rr trace.
Flags: needinfo?(emilio)
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.
Summary: stylo: Linux 64 hang in StyleSheet::GetCssRules in layout/style/test/chrome/test_stylesheet_clone_import_rule.html → stylo: Linux 64 / Windows 7 / Windows 8 hang in StyleSheet::GetCssRules in layout/style/test/chrome/test_stylesheet_clone_import_rule.html
Updated•7 years ago
|
Priority: -- → P3
Comment hidden (Intermittent Failures Robot) |
Comment 12•7 years ago
|
||
(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•7 years ago
|
||
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•7 years ago
|
||
jryans, could you try this test on asan and see if there is any special alert?
Flags: needinfo?(jryans)
Comment 15•7 years ago
|
||
This is probably one of the very few tests we are training the stylesheet cloning code I suppose. So bumping to P2.
Priority: P3 → P2
Comment 16•7 years ago
|
||
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.
Assignee: bwerth → xidorn+moz
Flags: needinfo?(jryans)
Comment 17•7 years ago
|
||
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 hidden (mozreview-request) |
Assignee | ||
Comment 19•7 years ago
|
||
Attachment 8902891 [details] is a patch of the test that turns timeouts into failures. I still can't reproduce on macOS.
Assignee | ||
Comment 20•7 years ago
|
||
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•7 years ago
|
||
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 hidden (mozreview-request) |
Assignee | ||
Comment 23•7 years ago
|
||
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...
Assignee | ||
Comment 24•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e95c5268d096a27a510f91e20a49e25f10d6f8d4
Assignee | ||
Comment 25•7 years ago
|
||
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•7 years ago
|
||
Assign back to Brad since it seems he's actively investigating.
Assignee: xidorn+moz → bwerth
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8905298 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8905297 -
Flags: review?(xidorn+moz)
Attachment #8902891 -
Flags: review?(xidorn+moz)
Assignee | ||
Comment 32•7 years ago
|
||
(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•7 years ago
|
||
mozreview-review |
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.
Attachment #8905297 -
Flags: review?(xidorn+moz) → review+
Comment 34•7 years ago
|
||
mozreview-review |
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
Attachment #8902891 -
Flags: review?(xidorn+moz) → review+
Comment 35•7 years ago
|
||
Pushed by bwerth@mozilla.com: 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/22af8e4748e3 https://hg.mozilla.org/mozilla-central/rev/865d5faef4a7
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 37•6 years ago
|
||
This didn't re-enable the test on stylo, was that intentional?
Flags: needinfo?(bwerth)
Comment 38•6 years ago
|
||
https://searchfox.org/mozilla-central/rev/b469db5c90d618f4b202d5ef280e1a78224eb43b/layout/style/test/chrome/chrome.ini#28
Assignee | ||
Comment 39•6 years ago
|
||
(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.
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(bwerth)
You need to log in
before you can comment on or make changes to this bug.
Description
•