Closed Bug 1410226 Opened 3 years ago Closed 3 years ago

stylo: Assertion failure: mOwner == ExpectedOwnerForChild(aFrame) (Missed some frame in the hierarchy?)

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- disabled
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: tsmith, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(2 files)

Attached file test_case.html
Assertion failure: mOwner == ExpectedOwnerForChild(aFrame) (Missed some frame in the hierarchy?), at /src/layout/base/ServoRestyleManager.cpp:154

#0 mozilla::ServoRestyleState::ChangesHandledFor(nsIFrame const&) const /src/layout/base/ServoRestyleManager.cpp:153:3
#1 mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:807:35
#2 mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:952:32
#3 mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:952:32
#4 mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:952:32
#5 mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:952:32
#6 mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:952:32
#7 mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:952:32
#8 mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:952:32
#9 mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:952:32
#10 mozilla::ServoRestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:1141:28
#11 mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /src/layout/base/PresShell.cpp:4147:41
#12 nsRefreshDriver::Tick(long, mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:1926:18
#13 mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /src/layout/base/nsRefreshDriver.cpp:307:7
#14 mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:329:5
#15 mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:770:5
#16 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:683:35
#17 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:584:9
#18 mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) /src/layout/ipc/VsyncChild.cpp:67:16
#19 mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) /src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:155:20
#20 mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:1755:28
#21 mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /src/ipc/glue/MessageChannel.cpp:2119:25
#22 mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /src/ipc/glue/MessageChannel.cpp:2049:17
#23 mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /src/ipc/glue/MessageChannel.cpp:1895:5
#24 mozilla::ipc::MessageChannel::MessageTask::Run() /src/ipc/glue/MessageChannel.cpp:1928:15
#25 nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1037:14
#26 NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:512:10
#27 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21
#28 MessageLoop::RunInternal() /src/ipc/chromium/src/base/message_loop.cc:326:10
#29 MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299:3
#30 nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:158:27
#31 XRE_RunAppShell() /src/toolkit/xre/nsEmbedFunctions.cpp:877:22
#32 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:269:9
#33 MessageLoop::RunInternal() /src/ipc/chromium/src/base/message_loop.cc:326:10
#34 MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299:3
#35 XRE_InitChildProcess(int, char**, XREChildData const*) /src/toolkit/xre/nsEmbedFunctions.cpp:703:34
#36 content_process_main(mozilla::Bootstrap*, int, char**) /src/browser/app/../../ipc/contentproc/plugin-container.cpp:63:30
#37 main /src/browser/app/nsBrowserApp.cpp:280:18
#38 __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
#39 _start (/home/user/workspace/browsers/m-c-1508348667-asan-debug/firefox+0x41ebe4)
Flags: in-testsuite?
INFO: Last good revision: 11de870775c1c9b395dade9d780afb4183c43fb4
INFO: First bad revision: 15fc474cbc387b9fc8326f18e968651f242587f2
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=11de870775c1c9b395dade9d780afb4183c43fb4&tochange=15fc474cbc387b9fc8326f18e968651f242587f2
Blocks: 1375674
Has Regression Range: --- → yes
Flags: needinfo?(emilio)
See Also: → 1402476
Summary: Assertion failure: mOwner == ExpectedOwnerForChild(aFrame) (Missed some frame in the hierarchy?) → stylo: Assertion failure: mOwner == ExpectedOwnerForChild(aFrame) (Missed some frame in the hierarchy?)
FWIW, that patch added the assertion, because it fixed a bug like this... That doesn't mean I can't take a look to it of course, will do tomorrow morning too.
This one is fun, this is display: contents messing the insertion point parent with XBL / Shadow DOM.
Assignee: nobody → emilio
Flags: needinfo?(emilio)
There's some repetition in the new code: GetPrimaryFrame ... GetDisplayContentsStyleFor ...
GetFlattenedTreeParent occurs twice.  Could we write it something like this instead:

  nsIContent* content = aContent;
  nsIFrame* frame;
  while (!(frame = content->GetPrimaryFrame())) {
    if (!GetDisplayContentsStyleFor(content)) {
      return nullptr;
    }
    content = content->GetFlattenedTreeParent();
    if (!content) {
      return nullptr;
    }
  }
  ...
Flags: needinfo?(emilio)
Sure thing.
Flags: needinfo?(emilio)
Comment on attachment 8920546 [details]
Bug 1410226: Properly compute the insertion point for a display: contents child in an XBL binding.

https://reviewboard.mozilla.org/r/191554/#review196816

Thanks!
Attachment #8920546 - Flags: review?(mats) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f8703e5ad323
Properly compute the insertion point for a display: contents child in an XBL binding. r=mats
Emilio, should we uplift this fix to Beta 57?
Flags: needinfo?(emilio)
Backed out for failing reftests layout/reftests/forms/legend/shadow-dom.html and layout/reftests/css-display/display-contents-generated-content-2.html:

https://hg.mozilla.org/integration/autoland/rev/98f4e20b334813ee19fa5de2cc1117d0dc9ae70c

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f8703e5ad323b63ee97e75b0dffb960d3cc9ca54&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry

For display-contents-generated-content-2.html the last light green span is shown/missing, for layout/reftests/forms/legend/shadow-dom.html the heading for box 6.
Argh, I ran display-contents-shadow-dom, but not those, thanks Aryx.

No, we don't need to uplift this, we've shipped this bug for a long time, it's just a stylo assertion that happens to catch it.
Oh, I'm stupid, this is of course because of the rewrite mats suggested, and I messed up.

I kept:

  if (frame->GetContent() != aContent)

But now it shouldn't really be checking aContent. Sigh.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2245ad9f35b3c0c51b6865b7af68b53d7a65841, with the fix, just in case... Will land tomorrow morning since the fix is just:

- if (frame->GetContent() != aContent) {
+ if (frame->GetContent() != content) {

Leaving ni? for that.
Alternatively, just use aContent throughout?
We don't really need the original value, and it's a bit of a foot-gun to have
both aContent/content variables like this, so when the function is short enough
I think it's OK to clobber the param, although I usually try to avoid that.

Either way is fine with me, but if you keep the local var then please name
it something more distinct from aContent to avoid it being misread.
(my bad of suggesting "content" in the first place)
Agreed... Can't think of a better name, so I'll just reuse aContent.
Flags: needinfo?(emilio)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cc74db25e470
Properly compute the insertion point for a display: contents child in an XBL binding. r=mats
https://hg.mozilla.org/mozilla-central/rev/cc74db25e470
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.