Closed
Bug 1487568
Opened 6 years ago
Closed 6 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)
Core
XBL
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-
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•6 years 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•6 years 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•6 years 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•6 years ago
|
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Comment 4•6 years ago
|
||
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•6 years ago
|
||
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
Updated•6 years ago
|
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
Comment 9•6 years ago
|
||
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
Updated•6 years ago
|
Flags: needinfo?(bgrinstead)
Comment 10•6 years 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•6 years 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•6 years 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•6 years 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•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Assignee | ||
Updated•6 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 15•6 years ago
|
||
Not sure, someone from layout team. Perhaps emilio, but I guess he might be rather busy with other stuff.
Flags: needinfo?(bugs)
Comment 16•6 years ago
|
||
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•6 years ago
|
||
When filing a follow up I found Bug 758695 which surfaces the same issue with removing the whitespace
See Also: → 758695
Comment 18•6 years 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•6 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•