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

RESOLVED FIXED in Firefox 64

Status

()

defect
P3
normal
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

Trunk
mozilla64
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

10 months ago
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-

Updated

10 months ago
Priority: -- → P3
Assignee

Comment 1

9 months ago
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
Assignee

Comment 2

9 months ago
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.
Assignee

Comment 3

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

Updated

9 months ago
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+

Comment 6

9 months ago
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

Comment 7

9 months ago
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

Comment 8

9 months ago
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)

Comment 10

9 months ago
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
Assignee

Comment 11

9 months ago
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.
Assignee

Comment 12

9 months ago
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>
Assignee

Comment 13

9 months ago
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)

Comment 14

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/12dd3b8e00c5
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Assignee

Updated

9 months ago
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.
Assignee

Comment 17

9 months ago
When filing a follow up I found Bug 758695 which surfaces the same issue with removing the whitespace
See Also: → 758695

Comment 18

9 months ago
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

Comment 19

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d23c41efdf7b
Status: REOPENED → RESOLVED
Closed: 9 months ago9 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.