Closed Bug 1461299 Opened 6 years ago Closed 6 years ago

Ensure mHoverNode / mActiveNode never point to unbound NAC

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla62

Tracking Flags:

Tracking

Status

firefox62

---

fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

Bug 1461299: Ensure the editor root node is marked as NAC even if BindToFrame fails. 6 years ago Emilio Cobos Álvarez (:emilio) 59 bytes, text/x-review-board-request	smaug : review+	Details
Bug 1461299: Make ESM not point to unbound NAC in the hover / active chain. 6 years ago Emilio Cobos Álvarez (:emilio) 59 bytes, text/x-review-board-request	smaug : review+	Details

Emilio Cobos Álvarez (:emilio)

Assignee

Description

•

6 years ago

When NAC is destroyed, we don't notify the event state manager. That may cause mHoverNode / mActiveNode to keep references to unbound elements and worse, keep state in the chain incorrectly.

This is preventing me from landing bug 1460101.

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 1

•

6 years ago

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

Comment hidden (mozreview-request)

We do the following:

  CreateRootNode();
  nsresult rv = BindToFrame(this);
  NS_ENSURE_SUCCESS(rv, rv);
  aElements.AppendElement(mRootNode);

That means that if BindToFrame fails, mRootNode will never be appended to
aElements, and thus will never get the native anonymous bits in [1].

BindToFrame should assert, but it indeed can fail due to it being bound to a
different frame in print preview, because nsCSSFrameConstructor's
ReplicateFixedFrames is a massive hack. :(

Just ensure the NAC bit is properly set for now...

[1]: https://searchfox.org/mozilla-central/rev/a85db9e29eb3f022dbaf8b9a6390ecbacf51e7dd/layout/base/nsCSSFrameConstructor.cpp#4193

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

Comment hidden (mozreview-request)

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

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 4

•

6 years ago

mozreview-review

Comment on attachment 8975524 [details]
Bug 1461299: Ensure the editor root node is marked as NAC even if BindToFrame fails.

https://reviewboard.mozilla.org/r/243780/#review249682

I guess. Hackish.

Attachment #8975524 - Flags: review?(bugs) → review+

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 5

•

6 years ago

mozreview-review

Comment on attachment 8975525 [details]
Bug 1461299: Make ESM not point to unbound NAC in the hover / active chain.

https://reviewboard.mozilla.org/r/243782/#review249700

Attachment #8975525 - Flags: review?(bugs) → review+

Pulsebot

Comment 6

•

6 years ago

Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48f01ac94137
Ensure the editor root node is marked as NAC even if BindToFrame fails. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/699e9816b7ff
Make ESM not point to unbound NAC in the hover / active chain. r=smaug

Cristian Brindusan [:cbrindusan]

Comment 7

•

6 years ago

Backed out 2 changesets (bug 1461299) for debug crashtest failures on crashtests/626014.xhtml

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/d76102e32327391dc69b83a3a4da0c2ea55255f3

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=699e9816b7ff862cf30db83e8f365cf50d02e90f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&selectedJob=178431306

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=178431306&repo=mozilla-inbound&lineNumber=28817

22:06:41     INFO -  REFTEST TEST-PASS | file:///Z:/task_1526333972/build/tests/reftest/tests/layout/forms/crashtests/626014.xhtml | (LOAD ONLY)
22:06:41     INFO -  REFTEST TEST-END | file:///Z:/task_1526333972/build/tests/reftest/tests/layout/forms/crashtests/626014.xhtml
22:06:41     INFO -  ++DOMWINDOW == 200 (11D6F800) [pid = 572] [serial = 4756] [outer = 05BC1540]
22:06:41    ERROR -  REFTEST TEST-UNEXPECTED-FAIL | file:///Z:/task_1526333972/build/tests/reftest/tests/layout/forms/crashtests/626014.xhtml | assertion count 1 is more than expected 0 assertions

Flags: needinfo?(emilio)

Pulsebot

Comment 8

•

6 years ago

Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a0ca81ddbce
Ensure the editor root node is marked as NAC even if BindToFrame fails. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe2b8fcd7a47
Make ESM not point to unbound NAC in the hover / active chain. r=smaug

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 9

•

6 years ago

Relanded with the assertion asserting what it was intended to assert (IsRootOfNativeAnonymousSubtree() instead of IsRootOfAnonymousSubtree()). The latter has an NS_ASSERTION which doesn't really hold if the element is never bound like in that test-case (or if the element is already unbound fwiw).

Flags: needinfo?(emilio)

Andrew McCreight [:mccr8]

Updated

•

6 years ago

Priority: -- → P2

Cristian Brindusan [:cbrindusan]

Comment 10

•

6 years ago

Backed out 3 changesets (bug 1461299, bug 1460101) for reftest failures on events/EventStateManager.cpp

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/9d8086740c860e8f7beeb890401b7d850d68f539

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=fe2b8fcd7a475b65fd5cf85921a58ff22868ac53&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&selectedJob=178456463

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=178456463&repo=mozilla-inbound&lineNumber=3205

[task 2018-05-14T23:59:27.560Z] 23:59:27     INFO - REFTEST TEST-START | file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-svgglyphs/svg-glyph-extents.html == file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-svgglyphs/svg-glyph-extents-ref.html
[task 2018-05-14T23:59:27.562Z] 23:59:27     INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-svgglyphs/svg-glyph-extents.html | 22 / 23 (95%)
[task 2018-05-14T23:59:27.580Z] 23:59:27     INFO - ++DOMWINDOW == 36 (0x7ff052d35400) [pid = 1338] [serial = 70] [outer = 0x7ff073099400]
[task 2018-05-14T23:59:27.900Z] 23:59:27     INFO - --DOMWINDOW == 35 (0x7ff052d3a800) [pid = 1338] [serial = 39] [outer = (nil)] [url = data:text/html;charset=UTF-8,%3C%21%2D%2DCLEAR%2D%2D%3E]
[task 2018-05-14T23:59:27.902Z] 23:59:27     INFO - --DOMWINDOW == 34 (0x7ff052d39400) [pid = 1338] [serial = 40] [outer = (nil)] [url = file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-svgglyphs/svg-glyph-objectpattern.svg]
[task 2018-05-14T23:59:27.904Z] 23:59:27     INFO - --DOMWINDOW == 33 (0x7ff052d31400) [pid = 1338] [serial = 41] [outer = (nil)] [url = file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-svgglyphs/svg-glyph-objectpattern-ref.svg]
[task 2018-05-14T23:59:27.904Z] 23:59:27     INFO - --DOMWINDOW == 32 (0x7ff05300f800) [pid = 1338] [serial = 42] [outer = (nil)] [url = data:text/html;charset=UTF-8,%3C%21%2D%2DCLEAR%2D%2D%3E]
[task 2018-05-14T23:59:27.907Z] 23:59:27     INFO - --DOMWINDOW == 31 (0x7ff052d3ec00) [pid = 1338] [serial = 36] [outer = (nil)] [url = data:text/html;charset=UTF-8,%3C%21%2D%2DCLEAR%2D%2D%3E]
[task 2018-05-14T23:59:27.908Z] 23:59:27     INFO - --DOMWINDOW == 30 (0x7ff052d36c00) [pid = 1338] [serial = 38] [outer = (nil)] [url = file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-svgglyphs/svg-glyph-objectgradient-zoom-ref.svg]
[task 2018-05-14T23:59:27.908Z] 23:59:27     INFO - --DOMWINDOW == 29 (0x7ff052d3cc00) [pid = 1338] [serial = 37] [outer = (nil)] [url = file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-svgglyphs/svg-glyph-objectgradient-zoom.svg]
[task 2018-05-14T23:59:27.915Z] 23:59:27     INFO - --DOMWINDOW == 28 (0x7ff052d39800) [pid = 1338] [serial = 44] [outer = (nil)] [url = file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-svgglyphs/clip-ref.html]
[task 2018-05-14T23:59:27.917Z] 23:59:27     INFO - --DOMWINDOW == 27 (0x7ff05300f000) [pid = 1338] [serial = 43] [outer = (nil)] [url = file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-svgglyphs/clip.html]
[task 2018-05-14T23:59:30.864Z] 23:59:30     INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-svgglyphs/svg-glyph-extents-ref.html | 22 / 23 (95%)
[task 2018-05-14T23:59:30.882Z] 23:59:30     INFO - ++DOMWINDOW == 28 (0x7ff051685800) [pid = 1338] [serial = 71] [outer = 0x7ff073099400]
[task 2018-05-14T23:59:31.077Z] 23:59:31     INFO - REFTEST TEST-PASS | file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-svgglyphs/svg-glyph-extents.html == file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-svgglyphs/svg-glyph-extents-ref.html | image comparison, max difference: 0, number of differing pixels: 0
[task 2018-05-14T23:59:31.079Z] 23:59:31     INFO - REFTEST TEST-END | file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-svgglyphs/svg-glyph-extents.html == file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-svgglyphs/svg-glyph-extents-ref.html
[task 2018-05-14T23:59:31.082Z] 23:59:31     INFO - ++DOMWINDOW == 29 (0x7ff051686800) [pid = 1338] [serial = 72] [outer = 0x7ff073099400]
[task 2018-05-14T23:59:31.128Z] 23:59:31     INFO - REFTEST INFO | Slowest test took 3287ms (file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/text-svgglyphs/svg-glyph-extents.html)
[task 2018-05-14T23:59:31.130Z] 23:59:31     INFO - REFTEST INFO | Total canvas count = 0
[task 2018-05-14T23:59:31.515Z] 23:59:31     INFO - JavaScript error: chrome://reftest/content/reftest.jsm, line 1554: NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIPropertyBag2.getPropertyAsAString]
[task 2018-05-14T23:59:31.517Z] 23:59:31     INFO - Assertion failure: aContent->IsRootOfNativeAnonymousSubtree(), at /builds/worker/workspace/build/src/dom/events/EventStateManager.cpp:5411

Flags: needinfo?(emilio)

Pulsebot

Comment 13

•

6 years ago

Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da4a57269fea
Ensure the editor root node is marked as NAC even if BindToFrame fails. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/69a0dda97ae5
Make ESM not point to unbound NAC in the hover / active chain. r=smaug

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 14

•

6 years ago

Sigh, <svg:use>

Flags: needinfo?(emilio)

Dorel Luca [:dluca]

Comment 15

•

6 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/da4a57269fea
https://hg.mozilla.org/mozilla-central/rev/69a0dda97ae5

Status: NEW → RESOLVED

Closed: 6 years ago

status-firefox62: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla62

Emilio Cobos Álvarez (:emilio)

Assignee

Updated

•

6 years ago

Updated

•

6 years ago

Depends on: 1536290

Luca Greco [:rpl] [:luca] [:lgreco]

Updated

•

5 years ago