Closed Bug 1487568 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: XBL, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(2 files)

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
Flags: qe-verify-
Priority: -- → P3
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
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.
For browser.xhtml, we have extra whitespace text nodes that appear inside of
mBoundElement, which was causing XBL content to be incorrectly dropped.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
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 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.
Attachment #9008091 - Flags: review+
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9139afaa8d77
Ignore whitespace only text nodes for XBL compatibility hack for unmatched children dropping content;r=smaug
Component: Toolbars and Customization → XBL
Product: Firefox → Core
Version: unspecified → Trunk
Pushed by dluca@mozilla.com:
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
Backout by dvarga@mozilla.com:
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
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
Flags: needinfo?(bgrinstead)
Backout by dluca@mozilla.com:
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
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.
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>
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]
Flags: needinfo?(bgrinstead) → needinfo?(bugs)
https://hg.mozilla.org/mozilla-central/rev/12dd3b8e00c5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Not sure, someone from layout team. Perhaps emilio, but I guess he might be rather busy with other stuff.
Flags: needinfo?(bugs)
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.
When filing a follow up I found Bug 758695 which surfaces the same issue with removing the whitespace
See Also: → 758695
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d23c41efdf7b
Ignore whitespace only text nodes for XBL compatibility hack for unmatched children dropping content;r=smaug
https://hg.mozilla.org/mozilla-central/rev/d23c41efdf7b
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.