Closed Bug 1398981 Opened 9 years ago Closed 8 years ago

186,800 instances of "stylo: Web Components not supported yet" emitted from dom/base/nsDocument.cpp during linux64 debug testing

Categories

(Core :: DOM: Core & HTML, defect, P3)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox59 --- fixed

People

(Reporter: erahm, Assigned: jessica)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

patch, v2.
129.30 KB, patch
Details | Diff | Splinter Review
> 186849 WARNING: stylo: Web Components not supported yet: file dom/base/nsDocument.cpp, line 6413 This warning [1] shows up in the following test suites: > 13609 - test-linux64/debug-mochitest-devtools-chrome-e10s-10 dt10 > 11612 - test-linux64/debug-mochitest-e10s-6 6 > 10471 - test-linux64/debug-web-platform-tests-e10s-6 wpt6 > 9894 - test-linux64/debug-mochitest-devtools-chrome-e10s-6 dt6 > 9522 - test-linux64/debug-web-platform-tests-reftests-e10s-4 Wr4 > 8839 - test-linux64/debug-mochitest-devtools-chrome-e10s-2 dt2 > 7259 - test-linux64/debug-web-platform-tests-reftests-e10s-1 Wr1 > 6922 - test-linux64/debug-web-platform-tests-reftests-e10s-5 Wr5 > 6438 - test-linux64/debug-web-platform-tests-reftests-e10s-3 Wr3 > 4882 - test-linux64/debug-mochitest-devtools-chrome-e10s-3 dt3 > 4808 - test-linux64/debug-mochitest-e10s-9 9 > 4684 - test-linux64/debug-web-platform-tests-reftests-e10s-2 Wr2 > 4383 - test-linux64/debug-mochitest-e10s-2 2 > 3894 - test-linux64/debug-web-platform-tests-e10s-8 wpt8 > 3657 - test-linux64/debug-mochitest-e10s-3 3 > 3268 - test-linux64/debug-mochitest-devtools-chrome-e10s-7 dt7 > 3139 - test-linux64/debug-mochitest-e10s-8 8 > 2740 - test-linux64/debug-mochitest-devtools-chrome-e10s-8 dt8 > 2730 - test-linux64/debug-web-platform-tests-e10s-1 wpt1 > 2672 - test-linux64/debug-web-platform-tests-e10s-11 wpt11 > 2579 - test-linux64/debug-mochitest-e10s-5 5 > 2570 - test-linux64/debug-web-platform-tests-reftests-e10s-6 Wr6 > 2562 - test-linux64/debug-web-platform-tests-e10s-9 wpt9 > 2520 - test-linux64/debug-web-platform-tests-e10s-5 wpt5 > 2495 - test-linux64/debug-web-platform-tests-e10s-12 wpt12 > 2388 - test-linux64/debug-mochitest-e10s-7 7 > 2376 - test-linux64/debug-web-platform-tests-e10s-10 wpt10 > 2256 - test-linux64/debug-web-platform-tests-e10s-3 wpt3 > 2160 - test-linux64/debug-mochitest-webgl-e10s-1 gl1 > 2113 - test-linux64/debug-mochitest-chrome-1 c1 > 2106 - test-linux64/debug-mochitest-e10s-10 10 > 2082 - test-linux64/debug-mochitest-webgl-e10s-2 gl2 > 1994 - test-linux64/debug-mochitest-devtools-chrome-e10s-1 dt1 > 1929 - test-linux64/debug-web-platform-tests-e10s-2 wpt2 > 1876 - test-linux64/debug-web-platform-tests-e10s-4 wpt4 > 1856 - test-linux64/debug-mochitest-webgl-e10s-3 gl3 > 1843 - test-linux64/debug-mochitest-clipboard-e10s cl > 1720 - test-linux64/debug-web-platform-tests-e10s-7 wpt7 > 1510 - test-linux64/debug-mochitest-chrome-3 c3 > 1428 - test-linux64/debug-mochitest-chrome-2 c2 > 1351 - test-linux64/debug-mochitest-browser-chrome-e10s-16 bc16 > 1350 - test-linux64/debug-mochitest-browser-chrome-e10s-15 bc15 > 1233 - test-linux64/debug-mochitest-devtools-chrome-e10s-4 dt4 > 1198 - test-linux64/debug-mochitest-e10s-1 1 > 1154 - test-linux64/debug-mochitest-browser-chrome-e10s-12 bc12 > 1138 - test-linux64/debug-mochitest-e10s-4 4 > 1088 - test-linux64/debug-mochitest-a11y a11y > 1062 - test-linux64/debug-test-verify-e10s TV > 950 - test-linux64/debug-mochitest-media-e10s-2 mda2 > 930 - test-linux64/debug-mochitest-media-e10s-1 mda1 > 904 - test-linux64/debug-mochitest-media-e10s-3 mda3 > 808 - test-linux64/debug-mochitest-browser-chrome-e10s-8 bc8 > 682 - test-linux64/debug-mochitest-browser-chrome-e10s-4 bc4 > 614 - test-linux64/debug-mochitest-browser-chrome-e10s-14 bc14 > 566 - test-linux64/debug-mochitest-browser-chrome-e10s-5 bc5 > 556 - test-linux64/debug-mochitest-browser-chrome-e10s-10 bc10 > 556 - test-linux64/debug-mochitest-browser-chrome-e10s-11 bc11 > 512 - test-linux64/debug-mochitest-browser-chrome-e10s-13 bc13 > 468 - test-linux64/debug-mochitest-devtools-chrome-e10s-9 dt9 > 430 - test-linux64/debug-mochitest-browser-chrome-e10s-2 bc2 > 294 - test-linux64/debug-mochitest-browser-chrome-e10s-6 bc6 > 282 - test-linux64/debug-mochitest-browser-chrome-e10s-7 bc7 > 246 - test-linux64/debug-mochitest-browser-chrome-e10s-9 bc9 > 187 - test-linux64/debug-mochitest-browser-chrome-e10s-1 bc1 > 184 - test-linux64/debug-mochitest-browser-chrome-e10s-3 bc3 > 76 - test-linux64/debug-reftest-e10s-8 R8 > 76 - test-linux64/debug-reftest-no-accel-e10s-8 Ru8 > 74 - test-linux64/debug-mochitest-gpu-e10s gpu > 50 - test-linux64/debug-mochitest-devtools-chrome-e10s-5 dt5 > 18 - test-linux64/debug-crashtest-e10s C > 6 - test-linux64/debug-reftest-no-accel-e10s-4 Ru4 > 6 - test-linux64/debug-reftest-e10s-4 R4 > 4 - test-linux64/debug-reftest-e10s-5 R5 > 4 - test-linux64/debug-reftest-no-accel-e10s-5 Ru5 > 2 - test-linux64/debug-reftest-e10s-6 R6 > 2 - test-linux64/debug-reftest-no-accel-e10s-6 Ru6 > 2 - test-linux64/debug-mochitest-jetpack JP It shows up in 33617 tests. A few of the most prevalent: > 1540 - [e10s] devtools/client/inspector/grids/test/browser_grids_restored-after-reload.js > 830 - [e10s] layout/style/test/test_media_queries.html > 792 - [e10s] dom/tests/browser/browser_noopener.js > 774 - [e10s] devtools/client/inspector/grids/test/browser_grids_grid-list-on-mutation-element-added.js > 768 - [e10s] /html/syntax/parsing/html5lib_tests16.html?run_type=NNNNNN > 681 - [e10s] devtools/client/inspector/grids/test/browser_grids_grid-list-on-iframe-reloaded.js > 672 - [e10s] layout/base/tests/test_reftests_with_caret.html > 636 - [e10s] layout/style/test/test_selectors.html > 634 - [e10s] devtools/client/inspector/grids/test/browser_grids_grid-outline-updates-on-grid-change.js > 634 - [e10s] devtools/client/inspector/grids/test/browser_grids_grid-list-on-mutation-element-removed.js [1] https://hg.mozilla.org/mozilla-central/annotate/f9a5e9ed6210/dom/base/nsDocument.cpp#l6413
Priority: -- → P4
We are not using P4 for bug triage, moving to P3 (backlog).
Priority: P4 → P3
Hm, so this code would only run if webcomponents were enabled. The fact that we're tripping this across lots of different test suites is concerning. I went looking, and found [1]. Andrew, does this mean we're running the entire test suite with web components enabled, despite the fact that we're not shipping it and it doesn't properly interact with things like stylo? If so, maybe we should stop doing that, and use more locally-scoped prefs for web-components changes? [1] http://searchfox.org/mozilla-central/rev/2c9a5993ac40ec1db8450e3e0a85702fa291b9e2/testing/profiles/prefs_general.js#68
Flags: needinfo?(overholt)
That seems wrong (not to mention I would prefer we scoped it to Shadow DOM and Custom Elements more explicitly). Jessica/John, I presume we'd like to continue running with dom.webcomponents.customelements.enabled=true for >= 58, right? smaug, I also presume we're ok with turning off the older dom.webcomponents.enabled for now (and/or setting up a separate dom.shadowdom.enabled pref?)? We're not shipping Shadow DOM or Custom Elements to release so I think it's ok to change the prefs as above but I'd like Jessica, John, and smaug's approval before we make that change. Bobby: note that we are aiming to ship Custom Elements soon (58, maybe? I don't want to get people too excited just yet) and Shadow DOM shortly thereafter (60? again, if you're reading this, no commitments). I was talking about this yesterday with Jet and I don't *think* there's any Stylo work for Custom Elements (or at least what we're going to ship initially) but there will be some for Shadow DOM.
Flags: needinfo?(overholt)
Flags: needinfo?(jjong)
Flags: needinfo?(jdai)
Flags: needinfo?(bugs)
(In reply to Andrew Overholt [:overholt] from comment #3) > I was > talking about this yesterday with Jet and I don't *think* there's any Stylo > work for Custom Elements (or at least what we're going to ship initially) > but there will be some for Shadow DOM. That matches my understanding. In theory we have a lot of the infrastructure for shadow DOM in place already, since we already need to handle NAC and XBL, and thus already go through all the FlattenedTree abstrations where appropriate. But I'd imagine some bugs will shake out. There's also the whole business about how selectors match inside of shadow trees, which will probably require some special handling.
(In reply to Andrew Overholt [:overholt] from comment #3) > Jessica/John, I presume we'd like to continue running with > dom.webcomponents.customelements.enabled=true for >= 58, right? > smaug, I also presume we're ok with turning off the older > dom.webcomponents.enabled for now (and/or setting up a separate > dom.shadowdom.enabled pref?)? Yes, if we're planning to ship Custom Elements in 58 (or later), then we should have dom.webcomponents.customelements.enabled=true for >= 58. We should remember to switch to use 'dom.webcomponents.customelements.enabled' for all custom elements related code before turning off 'dom.webcomponents.enabled'. I can help on that. > > We're not shipping Shadow DOM or Custom Elements to release so I think it's > ok to change the prefs as above but I'd like Jessica, John, and smaug's > approval before we make that change.
Flags: needinfo?(jjong)
(In reply to Jessica Jong [:jessica] from comment #5) > (In reply to Andrew Overholt [:overholt] from comment #3) > > > Jessica/John, I presume we'd like to continue running with > > dom.webcomponents.customelements.enabled=true for >= 58, right? > > smaug, I also presume we're ok with turning off the older > > dom.webcomponents.enabled for now (and/or setting up a separate > > dom.shadowdom.enabled pref?)? > Yes. I agree with that. > Yes, if we're planning to ship Custom Elements in 58 (or later), then we > should have dom.webcomponents.customelements.enabled=true for >= 58. > We should remember to switch to use > 'dom.webcomponents.customelements.enabled' for all custom elements related > code before turning off 'dom.webcomponents.enabled'. I can help on that. > File bug 1400762. > > > > We're not shipping Shadow DOM or Custom Elements to release so I think it's > > ok to change the prefs as above but I'd like Jessica, John, and smaug's > > approval before we make that change.
Flags: needinfo?(jdai)
I'll turn off 'dom.webcomponents.enabled' in this one and leave 'dom.webcomponents.customelements.enabled' on.
Assignee: nobody → jjong
Sounds good.
Flags: needinfo?(bugs)
This is taking longer than expected since I need to make all the tests that use Element.createShadowRoot()/getDestinationInsertionPoints()/shadowRoot to run in an iframe to make sure Element interface object is loaded with the correct value of the preference.
Attached patch patch, v1. (obsolete) — — Splinter Review
Attached patch patch, v1. (obsolete) — — Splinter Review
For wpt, we can just set the pref in the metadata file, but for mochitests, we need to set the pref and run the test in an iframe. Note that I disabled shadow dom related interfaces in test_interfaces.js.
Attachment #8915881 - Attachment is obsolete: true
Attachment #8915886 - Flags: review?(bugs)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfbb7af9a9569e7df8832609aa16b48a67d0cbd8&group_state=expanded&selectedJob=135311954 TV ( test-linux64/opt-test-verify-e10s / test-linux64/debug-test-verify-e10s ) still fails. :( For the fail cases, .createShadowRoot function is not found. Need to find out how the test is run first...
Hi Geoff, looks like the TV test suit tries to verify the modified tests by running them multiple times. I wonder if when verifying, it'd look at the 'skip-if' part in mochitest.ini, since the test that fails in [1] should be skipped if stylo is enabled. Another question is, how can I trigger TV test suit locally? Thanks. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfbb7af9a9569e7df8832609aa16b48a67d0cbd8&group_state=expanded&selectedJob=135311954
Flags: needinfo?(gbrown)
See Also: → 1406407
Sorry Jessica, I don't know why TV tried to run a skipped test; that's definitely not expected. I filed bug 1406407 to follow-up on that. To run locally, you can just add '--verify' to your mochitest (or reftest or xpcshell) command line: mach mochitest <your-test> --verify You can also push to try with '-u test-verify-e10s'. https://developer.mozilla.org/en-US/docs/Test_Verification might be helpful. By the way, I also notice that your try push had a "File exists" error for one of the tests. Just ignore that: It is being fixed in bug 1405369.
Flags: needinfo?(gbrown)
Comment on attachment 8915886 [details] [diff] [review] patch, v1. rs+ I wasn't aware of __dir__.ini before looking at this patch.
Attachment #8915886 - Flags: review?(bugs) → review+
(In reply to Geoff Brown [:gbrown] from comment #14) > Sorry Jessica, I don't know why TV tried to run a skipped test; that's > definitely not expected. I filed bug 1406407 to follow-up on that. > > To run locally, you can just add '--verify' to your mochitest (or reftest or > xpcshell) command line: > > mach mochitest <your-test> --verify > > You can also push to try with '-u test-verify-e10s'. > > https://developer.mozilla.org/en-US/docs/Test_Verification might be helpful. > > > By the way, I also notice that your try push had a "File exists" error for > one of the tests. Just ignore that: It is being fixed in bug 1405369. Thanks Geoff, it works fine after the latest patch in bug 1406407 has been merged.
Hi :eeejay, I'm trying to turn off webcomponents pref by default and turn it on only when running webcomponents related tests. However, I get an intermittent failure on 'accessible/tests/mochitest/hittest/test_shadowroot.html' due to this assertion: ' Assertion failure: container (Text node having rendered text hasn't accessible document!), at /builds/worker/workspace/build/src/accessible/base/NotificationController.cpp:721' (see https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb2840fb0f8021848f152a5c4d76c9d2c5a1d963&selectedJob=135640178) Do you know what could be wrong? Thanks.
Flags: needinfo?(eitan)
My guess is that you need webcomponents enabled! Passing this off to Alex since he knows this code better.
Flags: needinfo?(eitan) → needinfo?(surkov.alexander)
(In reply to Eitan Isaacson [:eeejay] from comment #18) > My guess is that you need webcomponents enabled! Passing this off to Alex > since he knows this code better. Oh, we do turn the preference on before running the test and we ran the test in an iframe for the pref to actually take effect, see: https://hg.mozilla.org/try/diff/41167b606222/accessible/tests/mochitest/hittest/test_shadowroot.html
See Also: → 1159768
(In reply to Jessica Jong [:jessica] from comment #19) > (In reply to Eitan Isaacson [:eeejay] from comment #18) > > My guess is that you need webcomponents enabled! Passing this off to Alex > > since he knows this code better. > > Oh, we do turn the preference on before running the test and we ran the test > in an iframe for the pref to actually take effect, see: > https://hg.mozilla.org/try/diff/41167b606222/accessible/tests/mochitest/ > hittest/test_shadowroot.html Not sure, it may point out to a bug in shadow DOM implementation: we traverse a DOM tree for that rendered text node by GetFlattenedTreeParent, I suspect it returns null at some point. So it's probably there's nothing wrong with the tests. Emilio, would you be willing to look at the problem, you might have ideas.
Flags: needinfo?(surkov.alexander) → needinfo?(emilio)
Is this bug relevant now the warning is gone and Stylo supports Webcomponents too? Or should we dupe this with bug 1409079?
Flags: needinfo?(emilio) → needinfo?(jjong)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #21) > Is this bug relevant now the warning is gone and Stylo supports > Webcomponents too? Or should we dupe this with bug 1409079? The bug itself is not valid anymore since we don't have these warning anymore after bug 1409079. But based on comment 2 and 3, it seems that we should enable the pref for related tests only. Although, I'm not sure if it worths making the change, now that we're resuming the implementation of Shadow DOM.
Flags: needinfo?(jjong)
(In reply to Jessica Jong [:jessica] from comment #22) > (In reply to Emilio Cobos Álvarez [:emilio] from comment #21) > > Is this bug relevant now the warning is gone and Stylo supports > > Webcomponents too? Or should we dupe this with bug 1409079? > > The bug itself is not valid anymore since we don't have these warning > anymore after bug 1409079. But based on comment 2 and 3, it seems that we > should enable the pref for related tests only. Although, I'm not sure if it > worths making the change, now that we're resuming the implementation of > Shadow DOM. Oh ok, in that case, it is probably a bug in the shadow DOM implementation. In particular, the only reason why that may return null is because the node is outside of the composed doc (in which case it shouldn't ever have a frame), or because it can't find the proper container, which it does looking at the flattened tree parent (so that may mean GetFlattenedTreeParent is messing up). Can you try a rebased patch? I added assertions in bug 1409088 which should catch the second bug, if it is that.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #23) > (In reply to Jessica Jong [:jessica] from comment #22) > > (In reply to Emilio Cobos Álvarez [:emilio] from comment #21) > > > Is this bug relevant now the warning is gone and Stylo supports > > > Webcomponents too? Or should we dupe this with bug 1409079? > > > > The bug itself is not valid anymore since we don't have these warning > > anymore after bug 1409079. But based on comment 2 and 3, it seems that we > > should enable the pref for related tests only. Although, I'm not sure if it > > worths making the change, now that we're resuming the implementation of > > Shadow DOM. > > Oh ok, in that case, it is probably a bug in the shadow DOM implementation. > > In particular, the only reason why that may return null is because the node > is outside of the composed doc (in which case it shouldn't ever have a > frame), or because it can't find the proper container, which it does looking > at the flattened tree parent (so that may mean GetFlattenedTreeParent is > messing up). > > Can you try a rebased patch? I added assertions in bug 1409088 which should > catch the second bug, if it is that. Thanks! I'll let you know the result once I get back to this.
status-firefox57=wontfix unless someone thinks this bug should block 57
status-firefox57: --- → wontfix
Attached patch patch, v2. — — Splinter Review
Rebased on top of the latest code base.
Attachment #8915886 - Attachment is obsolete: true
I still see this assertion when running try: Assertion failure: container (Text node having rendered text hasn't accessible document!), at /builds/worker/workspace/build/src/accessible/base/NotificationController.cpp:751 https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a756bd1ce54e6f523ed43bf418bf358fadfaf64&duplicate_jobs=visible&selectedJob=152847075
I tried with the latest codebase and got the same result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40735fab6e23085f7f5520681a4ee70ed0a06e9f Per comment 23, I didn't see any assertions added in bug 1409088, so maybe it's because "the node is outside of the composed doc (in which case it shouldn't ever have a frame), or because it can't find the proper container". Emilio, do you have any thoughts about this? Note that it started to fail after moving the test into an iframe.
Flags: needinfo?(emilio)
I'll take a look at that test.
(In reply to Jessica Jong [:jessica] from comment #28) > I tried with the latest codebase and got the same result: > > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=40735fab6e23085f7f5520681a4ee70ed0a06e9f > > Per comment 23, I didn't see any assertions added in bug 1409088, so maybe > it's because "the node is outside of the composed doc (in which case it > shouldn't ever have a frame), or because it can't find the proper > container". Emilio, do you have any thoughts about this? Note that it > started to fail after moving the test into an iframe. That assertion is in accessibility code. I bet we're inserting a child of a shadow root, and we're messing up. Opened bug 1427825 with a potential fix.
Flags: needinfo?(emilio)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #30) > (In reply to Jessica Jong [:jessica] from comment #28) > > I tried with the latest codebase and got the same result: > > > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=40735fab6e23085f7f5520681a4ee70ed0a06e9f > > > > Per comment 23, I didn't see any assertions added in bug 1409088, so maybe > > it's because "the node is outside of the composed doc (in which case it > > shouldn't ever have a frame), or because it can't find the proper > > container". Emilio, do you have any thoughts about this? Note that it > > started to fail after moving the test into an iframe. > > That assertion is in accessibility code. I bet we're inserting a child of a > shadow root, and we're messing up. Opened bug 1427825 with a potential fix. Here's a try run of the a11y tests with my patch on top of yours: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ecbceec037e53f0d85b4ae91087e6dd6320dae9
Thank you, Emilio, for looking into this and finding the fix in such a short time! :) I'll try this again once bug 1427825 is landed.
(In reply to Jessica Jong [:jessica] from comment #33) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=0b4fa962e9f5ac25e89b2a51f06f3d00daf9eafa&duplicate_job > s=visible The TV failure exists already in mozilla-inbound without the patch.
Pushed by jjong@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b647cd8f5436 Turn off webcomponents pref by default when running tests. r=smaug
https://hg.mozilla.org/mozilla-central/rev/b647cd8f5436
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Blocks: 1428685
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: