1487568 - browser.xhtml: Error when opening Customize Mode: TypeError: "lwthemeIcon is null, can't access property "style" of it"

Status: RESOLVED FIXED

(Core :: XBL, defect, P3)

Description

[Brian Grinstead [:bgrins]]

Note: this requires `mk_add_options 'export MOZ_BROWSER_XHTML=1'` to reproduce.

STR:
Right click on nav bar -> Customize
CustomizeMode: Error entering customize mode TypeError: "lwthemeIcon is null, can't access property "style" of it"
	_updateLWThemeButtonIcon resource:///modules/CustomizeMode.jsm:170:5
	enter resource:///modules/CustomizeMode.jsm:346:7
	enter resource:///modules/CustomizeMode.jsm:266:6
	setTab resource:///modules/CustomizeMode.jsm:203:7
	enter resource:///modules/CustomizeMode.jsm:237:7
	oncommand chrome://browser/content/browser.xhtml:1:1

Comment 1

[Brian Grinstead [:bgrins]]

This has something to do with XBL and whitespace:

- Only the <panel> in the main document gets inserted as a child [0], all of the anon content in the "menu" binding is removed
- If `<children includes="observes|template|menupopup|panel|tooltip"/>` is changed to just `<children />` it works [1]
- If it's changed to `<children includes="foo"/>` when we get the same behavior in XUL doc (the anonymous hbox doesn't get inserted)

[0]: https://searchfox.org/mozilla-central/rev/37663bb87004167184de6f2afa6b05875eb0528e/browser/components/customizableui/content/customizeMode.inc.xul#42
[1]: https://searchfox.org/mozilla-central/rev/37663bb87004167184de6f2afa6b05875eb0528e/toolkit/content/widgets/button.xml#213

Comment 2

[Brian Grinstead [:bgrins]]

It looks like we are hitting https://searchfox.org/mozilla-central/rev/9e7995b3c384945740985107a4de601b27904718/dom/xbl/nsXBLBinding.cpp#348-367:

// Compatibility hack. For some reason the original XBL
// implementation dropped the content of a binding if any child of
// the bound element didn't match any of the <children> in the
// binding. This became a pseudo-API that we have to maintain.

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: Bug 1487568 - Ignore whitespace only text nodes for XBL compatibility hack for unmatched children dropping content;r=smaug

For browser.xhtml, we have extra whitespace text nodes that appear inside of
mBoundElement, which was causing XBL content to be incorrectly dropped.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I've seen browser.xhtml mentioned a few times? What is that? Replacing browser.xul with .xhtml? How much slower or memory hungry is that to load? Feel free to explain on IRC.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9008091 [details]
Bug 1487568 - Ignore whitespace only text nodes for XBL compatibility hack for unmatched children dropping content;r=smaug

Olli Pettay [:smaug] has approved the revision.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9139afaa8d77
Ignore whitespace only text nodes for XBL compatibility hack for unmatched children dropping content;r=smaug

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12dd3b8e00c5
Ignore whitespace only text nodes for XBL compatibility hack for unmatched children dropping content;r=smaug

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6de80a4cd3ae
Backed out changeset 9139afaa8d77 for crashtest failure at tests/reftest/tests/layout/generic/crashtests/382745-1.xhtml on a CLOSED TREE

Comment 9

[Daniel Varga [:dvarga]]

Backed out changeset 9139afaa8d77 (Bug 1487568) for crashtest failure at tests/reftest/tests/layout/generic/crashtests/382745-1.xhtml on a CLOSED TREE

Push: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed,busted,exception&classifiedState=unclassified&revision=9139afaa8d7739f130d4457a5fd585cba55d2ee3

Failure log:

https://treeherder.mozilla.org/logviewer.html#?job_id=198918986&repo=mozilla-inbound&lineNumber=60768

[task 2018-09-12T19:28:26.955Z] 19:28:26     INFO - REFTEST TEST-PASS | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/382745-1.xhtml | (LOAD ONLY)
[task 2018-09-12T19:28:26.957Z] 19:28:26     INFO - REFTEST TEST-END | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/382745-1.xhtml
[task 2018-09-12T19:28:26.958Z] 19:28:26     INFO - ++DOMWINDOW == 128 (0x7fe0c5888000) [pid = 1108] [serial = 6578] [outer = 0x7fe117556800]
[task 2018-09-12T19:28:26.980Z] 19:28:26     INFO - REFTEST TEST-UNEXPECTED-FAIL | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/382745-1.xhtml | assertion count 1 is more than expected 0 assertions
[task 2018-09-12T19:28:26.981Z] 19:28:26     INFO - REFTEST TEST-START | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/383089-1.html
[task 2018-09-12T19:28:26.983Z] 19:28:26     INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/383089-1.html | 2043 / 3602 (56%)
[task 2018-09-12T19:28:27.004Z] 19:28:27     INFO - ++DOMWINDOW == 129 (0x7fe0f6985c00) [pid = 1108] [serial = 6579] [outer = 0x7fe117556800]
[task 2018-09-12T19:28:27.031Z] 19:28:27     INFO - [Child 1108, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /builds/worker/workspace/build/src/dom/base/nsContentUtils.cpp, line 4015
[task 2018-09-12T19:28:27.058Z] 19:28:27     INFO - --DOMWINDOW == 128 (0x7fe0fd5e8800) [pid = 1108] [serial = 6383] [outer = (nil)] [url = file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/337412-1.html]
[task 2018-09-12T19:28:27.059Z] 19:28:27     INFO - --DOMWINDOW == 127 (0x7fe0c53d8000) [pid = 1108] [serial = 6322] [outer = (nil)] [url = data:text/html;charset=UTF-8,%3C%21%2D%2DCLEAR%2D%2D%3E]
[task 2018-09-12T19:28:27.217Z] 19:28:27     INFO - REFTEST TEST-PASS | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/383089-1.html | (LOAD ONLY)
[task 2018-09-12T19:28:27.218Z] 19:28:27     INFO - REFTEST TEST-END | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/383089-1.html
[task 2018-09-12T19:28:27.221Z] 19:28:27     INFO - ++DOMWINDOW == 128 (0x7fe0c536e800) [pid = 1108] [serial = 6580] [outer = 0x7fe117556800]
[task 2018-09-12T19:28:27.242Z] 19:28:27     INFO - REFTEST TEST-START | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/385265-1.xhtml
[task 2018-09-12T19:28:27.244Z] 19:28:27     INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/385265-1.xhtml | 2044 / 3602 (56%)

Backout: https://hg.mozilla.org/integration/autoland/rev/6de80a4cd3aeafc145542b1bcb0ae0cca45abb01

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78b954d241ea
Backed out changeset 9139afaa8d77 for crashtest failure at tests/reftest/tests/layout/generic/crashtests/382745-1.xhtml on a CLOSED TREE

Comment 11

[Brian Grinstead [:bgrins]]

Hm, I guess this is due to us not dropping content from:

https://searchfox.org/mozilla-central/source/layout/generic/crashtests/382745-1.xhtml#4

Which means we don't drop <content> of:

https://searchfox.org/mozilla-central/source/layout/generic/crashtests/382745-1-binding.xml#3.

And we hit:

ASSERTION: Placeholder relationship should have been torn down already; this might mean we have a stray placeholder in the tree.: '!placeholder || nsLayoutUtils::IsProperAncestorFrame(aDestructRoot, placeholder)'

If I manually remove the <style> in the <content> then the assertion goes away.

Comment 12

[Brian Grinstead [:bgrins]]

Actually, I think the relevant node is the <treecols> https://searchfox.org/mozilla-central/source/layout/generic/crashtests/382745-1.xhtml#5 and the unmatched children includes from `includes="treecol|splitter"`: https://searchfox.org/mozilla-central/source/toolkit/content/widgets/tree.xml#937.

Which means the crashtest currently drops the <content> from the binding:

    <content orient="horizontal">
      <xul:hbox class="tree-scrollable-columns" flex="1">
        <children includes="treecol|splitter"/>
      </xul:hbox>
      <xul:treecolpicker class="treecol-image" fixed="true" xbl:inherits="tooltiptext=pickertooltiptext"/>
    </content>

So, I can reproduce this assertion on current m-c if I replace:

  <treecols>
    <treecol style="-moz-binding:url(382745-1-binding.xml#randomxbl);"/>
  </treecols>

With:

  <treecols><treecol style="-moz-binding:url(382745-1-binding.xml#randomxbl);"/></treecols>

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: updated-crashtest.patch

This causes the crashtest to fail on m-c. Smaug, with this applied I see the assertion. Could you help debug it or point me to someone who might be able to help?

 [Child 1108, Main Thread] ###!!! ASSERTION: Placeholder relationship should have been torn down already; this might mean we have a stray placeholder in the tree.: '!placeholder || nsLayoutUtils::IsProperAncestorFrame(aDestructRoot, placeholder)', file /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp, line 765
 #01: nsMenuPopupFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) [layout/xul/nsMenuPopupFrame.cpp:2315]

 #02: nsFrameList::DestroyFramesFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) [layout/generic/nsFrameList.cpp:127]

 #03: nsPopupSetFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) [mfbt/RefPtr.h:296]

 #04: nsFrameList::DestroyFramesFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) [layout/generic/nsFrameList.cpp:127]

 #05: nsContainerFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) [layout/generic/nsIFrame.h:2036]

 #06: nsDocElementBoxFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) [layout/xul/nsDocElementBoxFrame.cpp:80]

 #07: nsFrameList::DestroyFramesFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) [layout/generic/nsFrameList.cpp:127]

 #08: nsContainerFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) [layout/generic/nsIFrame.h:2036]

 #09: nsIFrame::Destroy() [layout/generic/nsIFrame.h:676]

 #10: nsContainerFrame::RemoveFrame(mozilla::layout::FrameChildListID, nsIFrame*) [layout/generic/nsContainerFrame.cpp:152]

 #11: nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) [layout/base/nsCSSFrameConstructor.cpp:7966]

 #12: nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, nsCSSFrameConstructor::InsertionKind) [layout/base/nsCSSFrameConstructor.cpp:9023]

 #13: mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList&) [layout/base/RestyleManager.cpp:1550]

 #14: mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) [xpcom/ds/nsTArray.h:372]

 #15: mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) [layout/base/PresShell.cpp:4298]

 #16: nsRefreshDriver::Tick(mozilla::TimeStamp) [layout/base/nsRefreshDriver.cpp:1908]

 #17: mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) [xpcom/ds/ArrayIterator.h:62]

 #18: mozilla::RefreshDriverTimer::Tick(mozilla::TimeStamp) [layout/base/nsRefreshDriver.cpp:320]

 #19: mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) [mfbt/RefPtr.h:79]

 #20: mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) [mfbt/RefPtr.h:42]

 #21: mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) [layout/ipc/VsyncChild.cpp:81]

 #22: mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) [s3:gecko-generated-sources:2c36fa176485b987fd1c1ce548d1f34c4c8bfdea36ff5dd016400feb13d3c5c0c7f99d5a56d13733937c9483a48617af010c09f521533a5ce0fc1f74c50b86a2/ipc/ipdl/PVsyncChild.cpp::167]

 #23: mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) [ipc/glue/MessageChannel.h:673]

 #24: mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) [ipc/glue/MessageChannel.cpp:2177]

 #25: mozilla::ipc::MessageChannel::MessageTask::Run() [xpcom/threads/Monitor.h:37]

 #26: nsThread::ProcessNextEvent(bool, bool*) [mfbt/Atomics.h:576]

 #27: NS_ProcessNextEvent(nsIThread*, bool) [xpcom/threads/nsThreadUtils.cpp:519]

 #28: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:97]

 #29: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:598]

 #30: nsBaseAppShell::Run() [widget/nsBaseAppShell.cpp:160]

 #31: XRE_RunAppShell() [toolkit/xre/nsEmbedFunctions.cpp:944]

 #32: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:269]

 #33: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:598]

 #34: XRE_InitChildProcess(int, char**, XREChildData const*) [toolkit/xre/nsEmbedFunctions.cpp:770]

 #35: content_process_main(mozilla::Bootstrap*, int, char**) [ipc/contentproc/plugin-container.cpp:51]

 #36: main [browser/app/nsBrowserApp.cpp:287]

Comment 14

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/12dd3b8e00c5

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Not sure, someone from layout team. Perhaps emilio, but I guess he might be rather busy with other stuff.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

So, what's happens here is that we have a frame tree that looks as follows:

Viewport
 - fixed block for the <tree style="position: fixed">
 - nsRootBoxFrame
   - nsDocElementBoxFrame
     - nsPopupSetFrame for the menupopup inside of the treecolpicker.
       - All the stuff in the popup.
     - nsPlaceholderFrame for the <tree>

And we're reframing the root, so we start destroying at nsRootBoxFrame.

Then the nsPopupSetFrame contains the <menupopup> itself, but its placeholder is inside the fixed block under the viewport. Note, crucially, how the nsPopupSetFrame has been inserted _before_ the placeholder for the <tree>, so when we destroy the <menupopup> under it, we still haven't destroyed the fixed block for it.

The good thing is that we will destroy it once we get around to destroy the placeholder, but this all sounds very fishy... I'm not sure how all this popupset stuff is supposed to be sound, but given this is behavior that has been here since forever, and it's not a correctness issue per se, since we'll actually get to destroy the stuff, it may be worth filing a bug and move on marking the test-case as asserts-if.

Comment 17

[Brian Grinstead [:bgrins]]

When filing a follow up I found Bug 758695 which surfaces the same issue with removing the whitespace

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d23c41efdf7b
Ignore whitespace only text nodes for XBL compatibility hack for unmatched children dropping content;r=smaug

Comment 19

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/d23c41efdf7b