Closed Bug 1436351 Opened 6 years ago Closed 6 years ago

JavaScript error: chrome://browser/content/tabbrowser.xml, line 2473: TypeError: cannot use 'in' operator to search for 'canGoBack' in 'browser'

Categories

(Firefox :: Tabbed Browser, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- fixed

People

(Reporter: ato, Assigned: timdream)

References

Details

(Keywords: regression)

Attachments

(1 file)

Since a few days ago there is a new JS error when running Marionette
tests (I haven’t checked if it applies in general) logged to stdout:

> JavaScript error: chrome://browser/content/tabbrowser.xml, line 2473: TypeError: cannot use 'in' operator to search for 'canGoBack' in 'browser'

This appears to be a regression of bug 1287330.
Depends on: 1287330
Flags: needinfo?(kevinhowjones)
Sorry, I was too quick on the trigger here.  I misread the date on
bug 1287330.  I don’t actually know what caused this.
No longer depends on: 1287330
Flags: needinfo?(kevinhowjones)
Keywords: regression
Depends on: 1430857
Bug 1430857 does not have anything to do with this, sorry.
No longer depends on: 1430857
(In reply to Andreas Tolfsen ‹:ato› from comment #0)
> > JavaScript error: chrome://browser/content/tabbrowser.xml, line 2473: TypeError: cannot use 'in' operator to search for 'canGoBack' in 'browser'

Which test is causing that, or every test in the unit test suite of Marionette? Any specific command we can run to see this error popping up?
Component: General → Tabbed Browser
Flags: needinfo?(ato)
Product: Toolkit → Firefox
It isn't test related, though that is where both the reporter and I noticed it.  It is related to lazy browsers.  You can see the error in console if you start firefox with multiple tabs.
Flags: needinfo?(ato)
Looks like aTab.linkedBrowser returns a non-object value...
Sigh.  hg bisect gave me...

The first bad revision is:
changeset:   402640:b3a81e6e2c1e
parent:      402639:a3612137cbd1
parent:      402622:0790ec12200d
user:        Gurzau Raul <rgurzau@mozilla.com>
date:        Wed Feb 07 00:00:20 2018 +0200
summary:     Merge mozilla-central to inbound.  a=merge CLOSED TREE
hg bisect --extend?
(In reply to Henrik Skupin (:whimboo) from comment #4)

> Which test is causing that, or every test in the unit test suite
> of Marionette? Any specific command we can run to see this error
> popping up?

I see it for every Mn test.  You can reproduce it this way:

> ./mach marionette test -z --gecko-log - testing/marionette/harness/marionette_harness/tests/unit/test_errors.py
> …
>  0:00.01 INFO Application command: /home/ato/src/gecko/obj-x86_64-pc-linux-gnu/dist/bin/firefox -no-remote -marionette -profile /tmp/tmpkdwnCi.mozrunner
> *** You are running in headless mode.
> JavaScript error: chrome://browser/content/tabbrowser.xml, line 2473: TypeError: cannot use 'in' operator to search for 'canGoBack' in 'browser'
> 1518099412486	Marionette	INFO	Listening on port 2828
Of course, as :mixedpuppy points out, it isn’t specifically
related to Mn.  (I didn’t think it was, but had no time to check.)
I guess it turns up in Mn a lot because the tests use a real browser.
This appears to happen due to the <tabs> constructor running before the <tabbrowser> constructor:

_insertBrowser@chrome://browser/content/tabbrowser.xml:2472:8
getRelatedElement@chrome://browser/content/tabbrowser.xml:7211:11
set_selectedIndex@chrome://global/content/bindings/tabbox.xml:386:31
tabs_XBL_Constructor@chrome://global/content/bindings/tabbox.xml:255:13

And then aTab.linkedBrowser is undefined in _insertBrowser.
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=eb72b3330274b2a5fdb36048d1b5c3c79f5aee7d&tochange=df6d2c3ee671e81f1a8b4f6d13197eee52db9680
Timothy Guan-tin Chien — Bug 1429464 - Remove toolbox binding r=Gijs

Confirmed via local backout.
Specifically, it's removing the binding from the toolbox that does it.
-moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbox");

If one gives toolbox a binding of chrome://global/content/bindings/toolbar.xml#toolbar-base, which is what the old binding extended, the error goes away.  It doesn't need to be that binding either, chrome://global/content/bindings/tabbox.xml#tab-base worked just as well, so :S.  Does having a binding change when stuff runs?
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Yeah, it's indeed related to timing. The error goes away with only an empty binding like

  <binding id="toolbox"></binding>
This is the line that sets the linkedBrowser on the tab, at function tabbrowser_XBL_constructor.

https://dxr.mozilla.org/mozilla-central/rev/0ac953fcddf10132eaecdb753d72b2ba5a43c32a/browser/base/content/tabbrowser.xml#5957

Without the binding this runs later than the part that throws.
Priority: -- → P1
I don't think I understand XUL enough yet to tell how the order of constructor call can change. In sum, this bug happens because <tabs> XBL constructor gets called before <tabbrowser> XBL constructor. The elements in question are all static in browser.xul in the following order:

<window>
  <toolbox> (removed binding was attached here)
    <toolbar>
      <tabs> (XBL constructor should be called later)
  <deck>
    <hbox>
      <vbox>
        <tabbrowser> (XBL constructor should be called earlier)

Related bindings are:

chrome://browser/content/tabbrowser.xml#tabbrowser-tabs defined in browser.css
chrome://global/content/bindings/tabbox.xml#tabs defined in xul.css

chrome://browser/content/tabbrowser.xml#tabbrowser defined in browser.css
Without trying to change the order of initialization. My current approach is trying to see if we could early return the _insertBrowser function instead, as the initial browser element should always be initialized by the tabbrowser.

Interestingly, the guard was there but it was removed in bug 1345090, with no apparent explanation:
https://searchfox.org/mozilla-central/diff/a7fa6eb9b7f3cd2fe3f3228ae447527ed13e284e/browser/base/content/tabbrowser.xml#2129

I doing a try push here to restore this !aTab.linkedBrowser check and see what would break. Will ask for review if this turned out alright.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3806aaeeb80ea2b0cd6f79780a87467274795972
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #16)
> Without trying to change the order of initialization.
> [...]
> Interestingly, the guard was there but it was removed in bug 1345090, with
> no apparent explanation:
> https://searchfox.org/mozilla-central/diff/
> a7fa6eb9b7f3cd2fe3f3228ae447527ed13e284e/browser/base/content/tabbrowser.
> xml#2129

The explanation is that aTab.linkedBrowser should basically never be null.

The backwards order (tabs initializing before tabbrowser, and tabbrowser._insertBrowser running before tabbrowser's constructor) breaks this, so I think this is where we need to look for a fix.
(In reply to Dão Gottwald [::dao] from comment #17)
> The backwards order (tabs initializing before tabbrowser, and
> tabbrowser._insertBrowser running before tabbrowser's constructor) breaks
> this, so I think this is where we need to look for a fix.

The problem is, the only quick "fix" here, I think, is to add an empty binding like comment 13. Unless we want to construct <tabs> (or one of its parent) dynamically, which will certainly sound the Talos alarm.

I did some black box testing on XBL constructor order; it looks like it does not follow the XML parsing order. With a binding on <toolbox>, this is what I see:

<window>
  <vbox id="titlebar"> (5)
  <toolbox> (3)
    <toolbar>
      <tabs> (4)
  <deck> (1)
    <hbox>
      <vbox>
        <tabbrowser> (2)

Without

<window>
  <vbox id="titlebar"> (4)
  <toolbox>
    <toolbar>
      <tabs> (1)
  <deck> (2)
    <hbox>
      <vbox>
        <tabbrowser> (3)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #18)
> (In reply to Dão Gottwald [::dao] from comment #17)
> > The backwards order (tabs initializing before tabbrowser, and
> > tabbrowser._insertBrowser running before tabbrowser's constructor) breaks
> > this, so I think this is where we need to look for a fix.
> 
> The problem is, the only quick "fix" here, I think, is to add an empty
> binding like comment 13.

I'd actually prefer this over a null-check. The assertion that tab.linkedBrowser must not be undefined is a good one, I wouldn't want to relax this and allow other code to violate it.

This would be a temporary hack that we should be able to remove after bug 1392352.
Comment on attachment 8950002 [details]
Bug 1436351 - Ensure tabs binding initialized after tabbrowser.

https://reviewboard.mozilla.org/r/219278/#review225020

::: browser/base/content/tabbrowser.xml:14
(Diff revision 1)
>            xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>            xmlns:xbl="http://www.mozilla.org/xbl">
>  
> +  <!--
> +  This binding is bound to <toolbox id="navigator-toolbox"> so that
> +  the #tabbrowser binding is initialized before the #tabs binding.

I would add: "Remove after bug 1392352."

::: browser/base/content/tabbrowser.xml:16
(Diff revision 1)
>  
> +  <!--
> +  This binding is bound to <toolbox id="navigator-toolbox"> so that
> +  the #tabbrowser binding is initialized before the #tabs binding.
> +  -->
> +  <binding id="empty"></binding>

<binding id="empty"/>
Attachment #8950002 - Flags: review?(dao+bmo) → review+
Comment on attachment 8950002 [details]
Bug 1436351 - Ensure tabs binding initialized after tabbrowser.

https://reviewboard.mozilla.org/r/219278/#review225022

::: browser/base/content/tabbrowser.xml:17
(Diff revisions 1 - 2)
>    <!--
>    This binding is bound to <toolbox id="navigator-toolbox"> so that
>    the #tabbrowser binding is initialized before the #tabs binding.
> +  Remove after bug 1392352.
>    -->
> -  <binding id="empty"></binding>
> +  <binding id="empty" />

style nit: remove the space before />

" />" was common back in the day for HTML that wanted to look like XML. This is real XML.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #18)
> (In reply to Dão Gottwald [::dao] from comment #17)
> > The backwards order (tabs initializing before tabbrowser, and
> > tabbrowser._insertBrowser running before tabbrowser's constructor) breaks
> > this, so I think this is where we need to look for a fix.
> 
> The problem is, the only quick "fix" here, I think, is to add an empty
> binding like comment 13. Unless we want to construct <tabs> (or one of its
> parent) dynamically, which will certainly sound the Talos alarm.
> 
> I did some black box testing on XBL constructor order; it looks like it does
> not follow the XML parsing order. With a binding on <toolbox>, this is what
> I see:
>  <snip>

Do we know why the ordering changed like this? Can we find out by logging / setting a debug breakpoint on the C++ code that calls the XBL constructor ? I kind of assume that there's JS that accesses the DOM node and forces the binding to be created. Neither of the orderings make much sense in terms of being triggered by purely DCL or similar.

(In reply to Dão Gottwald [::dao] from comment #19)
> This would be a temporary hack that we should be able to remove after bug
> 1392352.

Is Brian actively working on that bug? I think stopping to use XBL for tabbrowser is a relatively large project, so I'm a little uncomfortable adding a hack like this without a clear schedule for it going away, especially without understanding the fundamentals. I am additionally uncomfortable because we have run into what looks like a very similar problem in the recent past and it went undiagnosed that time, too: bug 1401846. It's not good if we have a pile of code underlying some of our core UI that we can't trust and where we are unable to fathom why it behaves the way it does, adding hacks until it does what we want it to.
Flags: needinfo?(timdream)
Flags: needinfo?(bgrinstead)
(In reply to :Gijs from comment #26)
> Do we know why the ordering changed like this? Can we find out by logging /
> setting a debug breakpoint on the C++ code that calls the XBL constructor ?
> I kind of assume that there's JS that accesses the DOM node and forces the
> binding to be created. Neither of the orderings make much sense in terms of
> being triggered by purely DCL or similar.

I assume that printing the stack with |try { throw new Error() } catch (e) { console.log(e.stack) }| will also print out the JS that forces the binding creation. I can dig deeper to see if this is the case.

> (In reply to Dão Gottwald [::dao] from comment #19)
> > This would be a temporary hack that we should be able to remove after bug
> > 1392352.
> 
> Is Brian actively working on that bug? I think stopping to use XBL for
> tabbrowser is a relatively large project, so I'm a little uncomfortable
> adding a hack like this without a clear schedule for it going away,
> especially without understanding the fundamentals. I am additionally
> uncomfortable because we have run into what looks like a very similar
> problem in the recent past and it went undiagnosed that time, too: bug
> 1401846. It's not good if we have a pile of code underlying some of our core
> UI that we can't trust and where we are unable to fathom why it behaves the
> way it does, adding hacks until it does what we want it to.
Flags: needinfo?(timdream)
(In reply to :Gijs from comment #26)
> Do we know why the ordering changed like this? Can we find out by logging /
> setting a debug breakpoint on the C++ code that calls the XBL constructor ?
> I kind of assume that there's JS that accesses the DOM node and forces the
> binding to be created. 

Without spinning up a debug build, I've tried to find any code that could access the DOM node through |gBrowser|. The only one that ran before tabs binding initialization is the urlbarBinding.xml#urlbar constructor 

https://dxr.mozilla.org/mozilla-central/rev/2b7d42d527af582a465e49187fe387442d524a7c/browser/base/content/urlbarBindings.xml#140

Yet comment out this line did not correct the initialization order. I'll try to investigate further with a debug build.

> Neither of the orderings make much sense in terms of
> being triggered by purely DCL or similar.

Do we know definitely how XBL binding should behave? I did a quick test this morning with an empty DOM and empty bindings look like it runs in a last-node-first parent-node-first fashion.
:'(

I don't think I can identify any JS calls from the C++ backtrace. I set the breakpoint at the place JS engine prints the TypeError, and this is what I get:

#0	0x000000010becfdda in js::ReportInNotObjectError(JSContext*, JS::Handle<JS::Value>, int, JS::Handle<JS::Value>, int) at /Users/timdream/Repositories/gecko/central2/js/src/vm/Interpreter.cpp:1717
#1	0x000000010beb8730 in Interpret(JSContext*, js::RunState&) at /Users/timdream/Repositories/gecko/central2/js/src/vm/Interpreter.cpp:2245
#2	0x000000010beb5a97 in js::RunScript(JSContext*, js::RunState&) at /Users/timdream/Repositories/gecko/central2/js/src/vm/Interpreter.cpp:423
#3	0x000000010becbddd in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) at /Users/timdream/Repositories/gecko/central2/js/src/vm/Interpreter.cpp:495
#4	0x000000010becc420 in InternalCall(JSContext*, js::AnyInvokeArgs const&) at /Users/timdream/Repositories/gecko/central2/js/src/vm/Interpreter.cpp:522
#5	0x000000010becc496 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) at /Users/timdream/Repositories/gecko/central2/js/src/vm/Interpreter.cpp:541
#6	0x000000010beccf8d in js::CallSetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>) at /Users/timdream/Repositories/gecko/central2/js/src/vm/Interpreter.cpp:670
#7	0x000000010c9706ec in SetExistingProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyResult>, JS::ObjectOpResult&) at /Users/timdream/Repositories/gecko/central2/js/src/vm/NativeObject.cpp:2756
#8	0x000000010c96fdd8 in bool js::NativeSetProperty<(js::QualifiedBool)1>(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) at /Users/timdream/Repositories/gecko/central2/js/src/vm/NativeObject.cpp:2784
#9	0x000000010c5639d5 in js::SetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) at /Users/timdream/Repositories/gecko/central2/js/src/vm/NativeObject.h:1647
#10	0x000000010bedba2e in SetPropertyOperation(JSContext*, JSOp, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::Handle<JS::Value>) at /Users/timdream/Repositories/gecko/central2/js/src/vm/Interpreter.cpp:270
#11	0x000000010bebef4d in Interpret(JSContext*, js::RunState&) at /Users/timdream/Repositories/gecko/central2/js/src/vm/Interpreter.cpp:2893
#12	0x000000010beb5a97 in js::RunScript(JSContext*, js::RunState&) at /Users/timdream/Repositories/gecko/central2/js/src/vm/Interpreter.cpp:423
#13	0x000000010becbddd in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) at /Users/timdream/Repositories/gecko/central2/js/src/vm/Interpreter.cpp:495
#14	0x000000010becc420 in InternalCall(JSContext*, js::AnyInvokeArgs const&) at /Users/timdream/Repositories/gecko/central2/js/src/vm/Interpreter.cpp:522
#15	0x000000010becc496 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) at /Users/timdream/Repositories/gecko/central2/js/src/vm/Interpreter.cpp:541
#16	0x000000010c564e1c in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) at /Users/timdream/Repositories/gecko/central2/js/src/jsapi.cpp:2978
#17	0x00000001083929dd in JS::Call(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) at /Users/timdream/Repositories/gecko/obj-x86_64-apple-darwin16.7.0-debug/dist/include/jsapi.h:3219
#18	0x00000001083928ba in nsXBLProtoImplAnonymousMethod::Execute(nsIContent*, JSAddonId*) at /Users/timdream/Repositories/gecko/central2/dom/xbl/nsXBLProtoImplMethod.cpp:330
#19	0x000000010837cd7c in nsXBLPrototypeBinding::BindingAttached(nsIContent*) at /Users/timdream/Repositories/gecko/central2/dom/xbl/nsXBLPrototypeBinding.cpp:265
#20	0x0000000108376ad2 in nsXBLBinding::ExecuteAttachedHandler() at /Users/timdream/Repositories/gecko/central2/dom/xbl/nsXBLBinding.cpp:632
#21	0x0000000108376aa8 in nsXBLBinding::ExecuteAttachedHandler() at /Users/timdream/Repositories/gecko/central2/dom/xbl/nsXBLBinding.cpp:629
#22	0x00000001083768df in nsBindingManager::ProcessAttachedQueueInternal(unsigned int) at /Users/timdream/Repositories/gecko/central2/dom/xbl/nsBindingManager.cpp:439
#23	0x0000000105ee09f0 in nsBindingManager::ProcessAttachedQueue(unsigned int) at /Users/timdream/Repositories/gecko/central2/dom/xbl/nsBindingManager.h:107
#24	0x0000000108c3ddab in XBLConstructorRunner::Run() at /Users/timdream/Repositories/gecko/central2/layout/base/PresShell.cpp:1724
#25	0x0000000105c15cee in nsContentUtils::AddScriptRunner(already_AddRefed<nsIRunnable>) at /Users/timdream/Repositories/gecko/central2/dom/base/nsContentUtils.cpp:5777
#26	0x0000000105c15d52 in nsContentUtils::AddScriptRunner(nsIRunnable*) at /Users/timdream/Repositories/gecko/central2/dom/base/nsContentUtils.cpp:5784
#27	0x0000000108c009f7 in mozilla::PresShell::Initialize(int, int) at /Users/timdream/Repositories/gecko/central2/layout/base/PresShell.cpp:1836
#28	0x000000010847025d in mozilla::dom::XULDocument::StartLayout() at /Users/timdream/Repositories/gecko/central2/dom/xul/XULDocument.cpp:1581
#29	0x0000000108475194 in mozilla::dom::XULDocument::DoneWalking() at /Users/timdream/Repositories/gecko/central2/dom/xul/XULDocument.cpp:2700
#30	0x0000000108475beb in mozilla::dom::XULDocument::StyleSheetLoaded(mozilla::StyleSheet*, bool, nsresult) at /Users/timdream/Repositories/gecko/central2/dom/xul/XULDocument.cpp:2804
#31	0x0000000108475c54 in non-virtual thunk to mozilla::dom::XULDocument::StyleSheetLoaded(mozilla::StyleSheet*, bool, nsresult) ()
#32	0x00000001089aea91 in mozilla::css::Loader::SheetComplete(mozilla::css::SheetLoadData*, nsresult) at /Users/timdream/Repositories/gecko/central2/layout/style/Loader.cpp:1774
#33	0x00000001089ae748 in mozilla::css::Loader::ParseSheet(nsTSubstring<char16_t> const&, mozilla::Span<unsigned char const, 18446744073709551615ul>, mozilla::css::SheetLoadData*, bool&) at /Users/timdream/Repositories/gecko/central2/layout/style/Loader.cpp:1736
#34	0x00000001089ac9b7 in mozilla::css::SheetLoadData::OnStreamComplete(nsIUnicharStreamLoader*, nsISupports*, nsresult, nsTSubstring<char16_t> const&) at /Users/timdream/Repositories/gecko/central2/layout/style/Loader.cpp:623
#35	0x00000001089ae7d2 in non-virtual thunk to mozilla::css::SheetLoadData::OnStreamComplete(nsIUnicharStreamLoader*, nsISupports*, nsresult, nsTSubstring<char16_t> const&) ()
#36	0x0000000103891bb8 in nsUnicharStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) at /Users/timdream/Repositories/gecko/central2/netwerk/base/nsUnicharStreamLoader.cpp:96
#37	0x00000001037b98c4 in nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, nsresult) at /Users/timdream/Repositories/gecko/central2/netwerk/base/nsBaseChannel.cpp:878
#38	0x00000001037b999d in non-virtual thunk to nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, nsresult) ()
#39	0x00000001037f1dfd in nsInputStreamPump::OnStateStop() at /Users/timdream/Repositories/gecko/central2/netwerk/base/nsInputStreamPump.cpp:708
#40	0x00000001037f0c89 in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) at /Users/timdream/Repositories/gecko/central2/netwerk/base/nsInputStreamPump.cpp:436
#41	0x00000001037f1f0c in non-virtual thunk to nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) ()
#42	0x0000000103608869 in nsInputStreamReadyEvent::Run() at /Users/timdream/Repositories/gecko/central2/xpcom/io/nsStreamUtils.cpp:102
#43	0x0000000103679063 in nsThread::ProcessNextEvent(bool, bool*) at /Users/timdream/Repositories/gecko/central2/xpcom/threads/nsThread.cpp:1040
#44	0x000000010369ab5c in NS_ProcessPendingEvents(nsIThread*, unsigned int) at /Users/timdream/Repositories/gecko/central2/xpcom/threads/nsThreadUtils.cpp:459
#45	0x0000000108711fbe in nsBaseAppShell::NativeEventCallback() at /Users/timdream/Repositories/gecko/central2/widget/nsBaseAppShell.cpp:98
#46	0x00000001087b2951 in nsAppShell::ProcessGeckoEvents(void*) at /Users/timdream/Repositories/gecko/central2/widget/cocoa/nsAppShell.mm:436
#47	0x00007fff95c8e321 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ ()
#48	0x00007fff95c6f21d in __CFRunLoopDoSources0 ()
#49	0x00007fff95c6e716 in __CFRunLoopRun ()
#50	0x00007fff95c6e114 in CFRunLoopRunSpecific ()
#51	0x00007fff951ceebc in RunCurrentEventLoopInMode ()
#52	0x00007fff951cecf1 in ReceiveNextEventCommon ()
#53	0x00007fff951ceb26 in _BlockUntilNextEventMatchingListInModeWithFilter ()
#54	0x00007fff93765a54 in _DPSNextEvent ()
#55	0x00007fff93ee17ee in -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] ()
#56	0x00000001087b1334 in ::-[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:](NSEventMask, NSDate *, NSString *, BOOL) at /Users/timdream/Repositories/gecko/central2/widget/cocoa/nsAppShell.mm:158
#57	0x00007fff9375a3db in -[NSApplication run] ()
#58	0x00000001087b3577 in nsAppShell::Run() at /Users/timdream/Repositories/gecko/central2/widget/cocoa/nsAppShell.mm:715
#59	0x000000010baa269b in nsAppStartup::Run() at /Users/timdream/Repositories/gecko/central2/toolkit/components/startup/nsAppStartup.cpp:288
#60	0x000000010bc46399 in XREMain::XRE_mainRun() at /Users/timdream/Repositories/gecko/central2/toolkit/xre/nsAppRunner.cpp:4673
#61	0x000000010bc478bc in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) at /Users/timdream/Repositories/gecko/central2/toolkit/xre/nsAppRunner.cpp:4808
#62	0x000000010bc4803c in XRE_main(int, char**, mozilla::BootstrapConfig const&) at /Users/timdream/Repositories/gecko/central2/toolkit/xre/nsAppRunner.cpp:4900
#63	0x000000010bc5b8d7 in mozilla::BootstrapImpl::XRE_main(int, char**, mozilla::BootstrapConfig const&) at /Users/timdream/Repositories/gecko/central2/toolkit/xre/Bootstrap.cpp:49
#64	0x0000000100001016 in do_main(int, char**, char**) at /Users/timdream/Repositories/gecko/central2/browser/app/nsBrowserApp.cpp:231
#65	0x0000000100000b0a in main at /Users/timdream/Repositories/gecko/central2/browser/app/nsBrowserApp.cpp:304
#66	0x0000000100000a24 in start ()
Noted the JS stack is:

0 _insertBrowser(aTab = [object XULElement]) ["chrome://browser/content/tabbrowser.xml":2475]
    this = [object XULElement]
1 getRelatedElement(aTab = [object XULElement]) ["chrome://browser/content/tabbrowser.xml":7214]
    this = [object XULElement]
2 set_selectedIndex(val = 0) ["chrome://global/content/bindings/tabbox.xml":388]
    this = [object XULElement]
3 tabs_XBL_Constructor() ["chrome://global/content/bindings/tabbox.xml":257]
    this = [object XULElement]
I can confirm that binding for <tabs> was added *after* <tabbrowser>, so it gets run earlier.
There are hundreds of binding being added and run so I ended up inserting printf() in the code.

==

 0:00.53 /Users/timdream/Repositories/gecko/obj-x86_64-apple-darwin16.7.0-debug/dist/NightlyDebug.app/Contents/MacOS/firefox -no-remote -foreground -profile /Users/timdream/Repositories/gecko/obj-x86_64-apple-darwin16.7.0-debug/tmp/scratch_user
[23217, Main Thread] WARNING: Last startup was detected as a crash.: file /Users/timdream/Repositories/gecko/central2/toolkit/components/startup/nsAppStartup.cpp, line 904
Unable to read VR Path Registry from /Users/timdream/Library/Application Support/OpenVR/.openvr/openvrpaths.vrpath
++DOCSHELL 0x10d916000 == 1 [pid = 23217] [id = {db12f3d2-7724-9e48-bfed-7e6166e27ff0}]
++DOMWINDOW == 1 (0x106b89090) [pid = 23217] [serial = 1] [outer = 0x0]
++DOMWINDOW == 2 (0x10b875800) [pid = 23217] [serial = 2] [outer = 0x106b89090]
++DOCSHELL 0x10f382000 == 2 [pid = 23217] [id = {2acc6946-cd4f-6948-bcd2-5e136ea578fb}]
++DOMWINDOW == 3 (0x106b8abb0) [pid = 23217] [serial = 3] [outer = 0x0]
++DOMWINDOW == 4 (0x10f171800) [pid = 23217] [serial = 4] [outer = 0x106b8abb0]
++DOCSHELL 0x133c2d000 == 3 [pid = 23217] [id = {ec81d3fe-1960-9248-8916-372bc540f665}]
++DOMWINDOW == 5 (0x133555660) [pid = 23217] [serial = 5] [outer = 0x0]
**add** tooltip
**add** menubar
**add** menupopup
**run** menupopup
**run** menubar
**run** tooltip
**add** window
++DOCSHELL 0x13500c800 == 4 [pid = 23217] [id = {f21ba0d0-aa2b-f340-8b3d-62eb58d140be}]
++DOMWINDOW == 6 (0x133558e90) [pid = 23217] [serial = 6] [outer = 0x0]
**add** tooltip
**add** toolbarbutton
**add** toolbarbutton
**add** toolbarbutton
**add** button
**add** vbox
**add** splitter
**add** notificationbox
**add** xul:browser
**add** xul:notificationbox
**add** xul:tabpanels
**add** xul:tabbox
**add** tabbrowser
**add** deck
**add** stringbundle
**add** stringbundle
**add** stringbundle
**add** menupopup
**add** menupopup
**add** tooltip
**add** tooltip
**add** panel
**add** panel
**add** panel
**add** panel
**add** menulist
**add** panel
**add** panel
**add** panel
**add** panel
**add** panel
**add** menupopup
**add** menupopup
**add** menupopup
**add** menupopup
**add** menupopup
**add** panel
**add** panel
**add** panel
**add** menupopup
**add** tooltip
**add** tooltip
**add** tooltip
**add** tooltip
**add** panel
**add** popupnotification
**add** popupnotification
**add** popupnotification
**add** popupnotification
**add** popupnotification
**add** popupnotification
**add** popupnotification
**add** popupnotification
**add** panel
**add** panel
**add** panel
**add** menupopup
**add** menupopup
**add** panel
**add** panel
**add** panel
**add** panel
**add** tooltip
**add** menupopup
**add** menupopup
**add** menubar
**add** toolbar
**add** xul:toolbarbutton
**add** xul:label
**add** tab
**add** xul:toolbarbutton
**add** xul:toolbarbutton
**add** xul:scrollbox
**add** xul:toolbarbutton
**add** xul:arrowscrollbox
**add** tabs
**add** toolbarbutton
**add** menupopup
**add** xul:dropmarker
**add** toolbarbutton
**add** button
**add** toolbar
**add** toolbarbutton
**add** toolbarbutton
**add** toolbarbutton
**add** toolbarbutton
**add** toolbarspring
**add** toolbarspring
**add** toolbarbutton
**add** toolbarbutton
**add** toolbarbutton
**add** toolbarbutton
**add** html:input
**add** xul:menupopup
**add** xul:hbox
**add** xul:dropmarker
**add** toolbarbutton
**add** textbox
**add** xul:label
**add** toolbarbutton
**add** toolbar
**add** toolbarbutton
**add** toolbarbutton
**add** scrollbox
**add** menupopup
**add** xul:dropmarker
**add** toolbarbutton
**add** toolbar
**add** notificationbox
**run** notificationbox
**run** toolbar
**run** toolbarbutton
**run** xul:dropmarker
**run** menupopup
**run** scrollbox
**run** toolbarbutton
**run** toolbarbutton
**run** toolbar
**add** toolbarbutton
**run** toolbarbutton
**run** toolbarbutton
**run** xul:label
**run** textbox
**run** toolbarbutton
**run** xul:dropmarker
**run** xul:hbox
**run** xul:menupopup
**run** html:input
**run** toolbarbutton
**run** toolbarbutton
**run** toolbarbutton
**run** toolbarbutton
**run** toolbarspring
**run** toolbarspring
**run** toolbarbutton
**run** toolbarbutton
**run** toolbarbutton
**run** toolbarbutton
**run** toolbar
**run** button
**run** toolbarbutton
**run** xul:dropmarker
**run** menupopup
**run** toolbarbutton
**run** tabs
JavaScript error: chrome://browser/content/tabbrowser.xml, line 2475: TypeError: cannot use 'in' operator to search for 'canGoBack' in 'browser'
**add** menupopup
**add** menupopup
**run** menupopup
**run** menupopup
**run** xul:arrowscrollbox
**run** xul:toolbarbutton
**run** xul:scrollbox
**run** xul:toolbarbutton
**run** xul:toolbarbutton
**run** tab
**run** xul:label
**run** xul:toolbarbutton
**run** toolbar
**run** menubar
**run** menupopup
**run** menupopup
**run** tooltip
**run** panel
**run** panel
**run** panel
**run** panel
**run** menupopup
**run** menupopup
**run** panel
**run** panel
**run** panel
**run** popupnotification
**run** popupnotification
**run** popupnotification
**run** popupnotification
**run** popupnotification
**run** popupnotification
**run** popupnotification
**run** popupnotification
**run** panel
**run** tooltip
**run** tooltip
**run** tooltip
**run** tooltip
**run** menupopup
**run** panel
**run** panel
**run** panel
**run** menupopup
**run** menupopup
**run** menupopup
**run** menupopup
**run** menupopup
**run** panel
**run** panel
**run** panel
**run** panel
**run** panel
**run** menulist
**run** panel
**run** panel
**run** panel
**run** panel
**run** tooltip
**run** tooltip
**run** menupopup
**run** menupopup
**run** stringbundle
**run** stringbundle
**run** stringbundle
**run** deck
**run** tabbrowser
**add** panel
**run** panel
**run** xul:tabbox
**run** xul:tabpanels
**run** xul:notificationbox
**run** xul:browser
++DOMWINDOW == 7 (0x134ffac00) [pid = 23217] [serial = 7] [outer = 0x133558e90]
**run** notificationbox
**run** splitter
**run** vbox
**run** button
**run** toolbarbutton
**run** toolbarbutton
**run** toolbarbutton
**run** tooltip
**run** window
This seems very similar to the condition race I was hunting around 57 timeline - bug 1401846 (which eventually blocked me from landing bug 1397365).
Flags: needinfo?(bgrinstead)
See Also: → 1401846
(In reply to :Gijs from comment #26)
> (In reply to Dão Gottwald [::dao] from comment #19)
> > This would be a temporary hack that we should be able to remove after bug
> > 1392352.
> 
> Is Brian actively working on that bug? I think stopping to use XBL for
> tabbrowser is a relatively large project, so I'm a little uncomfortable
> adding a hack like this without a clear schedule for it going away,
> especially without understanding the fundamentals. I am additionally
> uncomfortable because we have run into what looks like a very similar
> problem in the recent past and it went undiagnosed that time, too: bug
> 1401846. It's not good if we have a pile of code underlying some of our core
> UI that we can't trust and where we are unable to fathom why it behaves the
> way it does, adding hacks until it does what we want it to.

I've been waiting for Bug 1412456 to land before picking up the TabBrowser JS class conversion again, since some of that patch queue is working around addon interposition and AIUI that is going away  (as explained in https://bugzilla.mozilla.org/show_bug.cgi?id=1392352#c51). Kris, do you have an idea on the timeline for addon interposition being removed? It'd be helpful to know to plan the tabbrowser work - we could pick that queue back up with the interposition changes if it's not going to be removed soon.
Flags: needinfo?(kmaglione+bmo)
(In reply to :Gijs from comment #26)
> (In reply to Dão Gottwald [::dao] from comment #19)
> > This would be a temporary hack that we should be able to remove after bug
> > 1392352.
> 
> Is Brian actively working on that bug? I think stopping to use XBL for
> tabbrowser is a relatively large project, so I'm a little uncomfortable
> adding a hack like this without a clear schedule for it going away,
> especially without understanding the fundamentals. I am additionally
> uncomfortable because we have run into what looks like a very similar
> problem in the recent past and it went undiagnosed that time, too: bug
> 1401846. It's not good if we have a pile of code underlying some of our core
> UI that we can't trust and where we are unable to fathom why it behaves the
> way it does, adding hacks until it does what we want it to.

And FWIW, I remember seeing similar issues with the JS class conversion (i.e. https://bugzilla.mozilla.org/show_bug.cgi?id=1392352#c40), where the <tabs> XBL constructor runs during the TabBrowser class constructor and calls into a method on the class before the TabBrowser constructor has finished running, triggering very similar errors to what we see in this bug (since the TabBrowser isn't yet fully initialized). So I don't think that converting <tabbrowser> alone will fix the race, but it may at least make it perma-orange.
(In reply to Brian Grinstead [:bgrins] from comment #34)
> And FWIW, I remember seeing similar issues with the JS class conversion
> (i.e. https://bugzilla.mozilla.org/show_bug.cgi?id=1392352#c40), where the
> <tabs> XBL constructor runs during the TabBrowser class constructor and
> calls into a method on the class before the TabBrowser constructor has
> finished running, triggering very similar errors to what we see in this bug
> (since the TabBrowser isn't yet fully initialized). So I don't think that
> converting <tabbrowser> alone will fix the race, but it may at least make it
> perma-orange.

I should note - that particular issue is happening in the case where there is still a tabbrowser XBL binding with the same `<content>` and code like: `<constructor>gBrowser = new TabBrowser(this);</constructor>`. I don't think the issue would go away if we got rid of the binding entirely, but tbh I'm not really sure.
OK. So, for posterity, Tim and I ended up doing what you do if you get stuck lost in XBL/XUL land - I asked bz. Full discussion at https://mozilla.logbot.info/developers/20180212#c14284909 and further.

A somewhat condensed summary:

- the style sheet service does breadth first traversal of nodes and builds a stack/queue (more confusion there) of nodes whose constructors need running
- those items eventually get run in reverse tree order (so e.g. document.documentElement.lastChild will construct before document.documentElement.firstChild )
- once the traversal hits children that have ancestors that have bindings, those children are inserted in the list adjacent to their binding-having-ancestors.
- if traversal hits items that don't have ancestors with bindings, they get appended to the end of the list, which ends up meaning they run *first* (see stack/queue confusion mentioned before).

The issue here is that before the removal of the toolbox binding, traversal went roughly like this (liberally adapting from bz's analysis, as a function of node -> queue of items to run):

window -> ()
#titlebar -> (#titlebar)
toolbox -> (toolbox, #titlebar)
deck -> (deck, toolbox, #titlebar)
toolbar -> (deck, toolbox, toolbar, #titlebar)
tabs -> (deck, toolbox, toolbar, tabs, #titlebar)
hbox -> (deck, toolbox, toolbar, tabs, #titlebar)
vbox -> (deck, toolbox, toolbar, tabs, #titlebar)
tabbrowser -> (deck, tabbrowser, toolbox, toolbar, tabs, #titlebar)


Now, instead, we get:

window -> ()
#titlebar -> (#titlebar)
toolbox -> (#titlebar)
deck -> (deck, #titlebar)
toolbar -> (toolbar, deck, #titlebar)
tabs -> (toolbar, tabs, deck, #titlebar)
hbox -> (toolbar, tabs, deck, #titlebar)
vbox -> (toolbar, tabs, deck, #titlebar)
tabbrowser -> (toolbar, tabs, deck, tabbrowser, #titlebar)

This is all kinds of weird but never got changed because backwards compatibility:
a) parents get constructed before children, which is the opposite of what you'd expect
b) things get constructed breadth-first, which is very unusual for anything DOM-related
c) things get constructed last-before-first, which is also counterintuitive.

The only other way to try to influence this stuff is via JS, which can cause bindings to be created "early" - but only as long as they haven't already been added to these stacks/queues/whatever.


So options include:

not actually an option: try to force the tabbrowser constructor to run when the tabs constructor runs (by accessing tabbrowser from the tabs constructor). This is not possible because when the tabs constructor runs the tabbrowser constructor is already queued up, and we no-op here: https://searchfox.org/mozilla-central/rev/b9f1a4ecba48b2d8c686669e32d109c40e927b48/dom/base/Element.cpp#583-586

0) add some JS that runs before the frame constructors cause binding construction, explicitly constructing things from JS. This seems fraught with other risks related to the DOM / styling not having loaded.
1) add a no-op toolbox binding as the patch does
2) remove the binding for <deck> . It's not clear why that's even a binding. Then we'd hopefully end up with:
window -> ()
#titlebar -> (#titlebar)
toolbox -> (#titlebar)
deck -> (#titlebar)
toolbar -> (toolbar, #titlebar)
tabs -> (toolbar, tabs, #titlebar)
hbox -> (toolbar, tabs, #titlebar)
vbox -> (toolbar, tabs, #titlebar)
tabbrowser -> (tabbrowser, toolbar, tabs, #titlebar)

... though like adding the toolbox binding, this merely restores a somewhat unreliable equilibrium.

3) take all the tabbrowser/tabs stuff out of xbl as discussed.
4) come up with some kind of hacky attribute thing on the root element that core can use to deterministically queue up constructor instantiation for those elements first (would require hacking XBL a bit, but probably doable).
5) try to fix all of XBL. Nobody wants this.

For now, we can definitely go with (1), and maybe file bugs for (2) (which would allow removing the toolbox binding again) and/or prioritize (3). If we think (3) will take a long while, perhaps it's worth looking at (4).
Comment on attachment 8950002 [details]
Bug 1436351 - Ensure tabs binding initialized after tabbrowser.

https://reviewboard.mozilla.org/r/219278/#review225326
Attachment #8950002 - Flags: review?(gijskruitbosch+bugs) → review+
See Also: → 1437638
Pushed by timdream@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2d7c898128db
Ensure tabs binding initialized after tabbrowser. r=dao,Gijs
https://hg.mozilla.org/mozilla-central/rev/2d7c898128db
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
See Also: → 1456728
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: