1260763 - Some in-content preferences panes are not showing

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

rfeeley saw this, and I'm filing on his behalf.

He seemed to get into a state where some panes (applications, security, sync, content) will not be switched to, or will only show a blank pane.

I opened up his Browser Console and here's what I saw after attempting to switch to the content pane a few times:

Error initializing preference category paneContent: TypeError: preference.setElementValue is not a 
preferences.js:133
TypeError: preference.setElementValue is not a function
content.js:221:9
Error initializing preference category paneContent: TypeError: preference.setElementValue is not a 
preferences.js:133
TypeError: preference.setElementValue is not a function
content.js:221:9
Error initializing preference category paneContent: TypeError: preference.setElementValue is not a 
preferences.js:133
TypeError: preference.setElementValue is not a function
content.js:221:9
Error initializing preference category paneContent: TypeError: preference.setElementValue is not a 
preferences.js:133
TypeError: preference.setElementValue is not a function

Which suggests to me that somewhere, somehow the preference XBL binding isn't being attached to the thing we're calling setElementValue on.

rfeeley says he's seen this a few times, and that it might have something to do with having an unverified sync account, since that's what he was testing when he started seeing this.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

jaws, have you heard of anything like this before?

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

I haven't heard of it before.

I see on IRC he says he's using Nightly 48.0a1 (2016-03-24). Ryan, does it reproduce reliably enough if he opens the preferences < 10 times? Can you use mozregression?

Comment 3

[Ryan Feeley [:rfeeley]]

While I was in that state, which has happened twice, I absolutely could reproduce it 10x or more. But I am not sure how to re-enter that state or what mozregression is.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Okay, I've got some STR here, and I can reproduce it reliably.

1) Start Nightly, and browse the initial tab to about:newtab or something
2) Open DevTools, and then make them open in their own window.
3) In the Inspector tool, inspect some node by clicking on the pointer-selector icon thing in the top left of the devtools window, and inspect some node within about:newtab
4) Click on the menu button
5) Click on the Sign in to Sync button

ER:

about:preferences should open and display some stuff

AR:

about:preferences opens but a bunch of panels are just flat-out empty, as described in comment 0.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

It looks as if the Inspector is somehow interfering with the construction of the pref panes.

jryans, any ideas what's going on?

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I spent some time looking at this today, it's quite strange!

I was able to use a slightly different STR (only one page involved):

1. Start Nightly
2. Open about:preferences#sync
3. It should appear correctly
4. Open the DevTools (docked however you prefer)
5. Switch to either the Inspector or Style Editor tool
6. Refresh the page
7. Sync content no longer appears

If you look inside the <prefpane> with the Inspector, all of content.xul, security.xul, and sync.xul are missing from the DOM.  (These files are #included into preferences.xul at build time, so it is quite odd to see their content missing.)  applications.xul is also mostly missing, but first script tag inside it "chrome://browser/content/preferences/in-content/applications.js" is present and is the last element in the <prefpane>.

For the moment, I don't _think_ it's anything specific about applications.js...  If I remove "#include applications.xul", we block on some other JS file inside the <prefpane> instead.

The connection to Inspector and Style Editor appears to be that they both start the WalkerActor, and this calls `eventListenerService.addListenerChangeListener` on init[1].  If I comment out `eventListenerService.addListenerChangeListener`, the page works correctly with the DevTools.

:jdescottes, since you added this behavior in bug 1157469, can you take a look?

[1]: https://dxr.mozilla.org/mozilla-central/rev/ae7413abfa4d3954a6a4ce7c1613a7100f367f9a/devtools/server/actors/inspector.js#1355

Comment 7

[Julian Descottes [:jdescottes]]

Our "listenerChangeListener" gets called every time a new listener is added/removed. 
I tried emptying the callback -> this removes the issue.

For every changeListener we get, we retrieve the target element. (which is a nsIDOMEventTarget)
If we stop accessing this element (ie stop before "let target = current.target;") -> this removes the issue.
There is no error we can catch here. I even rebuilt in debug mode, but did not find any more log.

Then I managed to identify precisely which listener change was causing the issue. It is an onchange listener defined in a XUL file, on a textbox. In advanced.xul, we have :

>          <textbox id="cacheSize" type="number" size="4" max="1024"
>                   onchange="gAdvancedPane.updateCacheSizePref();"
>                   aria-labelledby="useCacheBefore cacheSize useCacheAfter"/>

If we remove the onchange event, all the preference pages can be reloaded without any issue.

We actually get a second event for the same element right after, this time accessing current.target does not trigger any issue. I think the first one corresponds to the event listener being removed when leaving the page, the second one corresponds to the event being added in the new page.

But if you go to the "Advanced" panel after getting a blank page, this panel will be correctly displayed...

Keeping the ni? for now, will investigate more.

Comment 8

[Julian Descottes [:jdescottes]]

Spent more time on this today.

In my previous comment I mentioned the issue was triggered by a specific textbox which had on onchange event attached in XUL. It turns out the issue occurs if:
- there is a textbox with an event listener defined in XUL (can be onclick, onchange, any event works)
- this textbox is in a preference pane included BEFORE the pref pane you are trying to view

Here the textbox is defined in advanced.xul. If we look at how the preference panes are included :
>
>       <prefpane id="mainPrefPane">
> #include main.xul
> #include search.xul
> #include privacy.xul
> #include advanced.xul
> #include applications.xul
> #include content.xul
> #include security.xul
> #include sync.xul
>       </prefpane>
>

"advanced.xul" is included before applications, content, security and sync. And these are the 4 panels that are failing when trying to reload them with the inspector open.
Moving "advanced.xul" at the end of this list works fixes the issue (but this is a workaround at best).

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: bug1260763.testcase.patch

This patch contains a simplified version of the preference pane and allows to reproduce the issue without using the devtools.

Here the "listenerChangeListener" is added in preferences.js, and is simply reading the "target" property of each event change.

The preferences only contain 3 dummy tabs : 
- main : contains just a label
- sync : contains just a label
- advanced : contains a label AND the famous textbox with an onchange event

The preference panes are included in the following order : main, advanced, sync.

STRs:
- import the patch
- open about:preferences
- click on "Sync"

AR: Panel is empty
ER: Panel should display the label "SYNC PANEL"

If you update the #include order of the XUL preference panes in preferences.xul to put "advanced.xul" after "sync.xul", the SYNC panel will be correctly displayed.

mconley, jryans: I isolated the issue as much as I could but I don't know how to proceed from there. Can you have a look at what I found, at my reduced test case and let me know if have any idea for a next step?

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: bug1260763.workaround.patch

The root cause of this bug is still under investigation. In the meantime, we *could* use this workaround to fix the symptom.

By putting including advanced.xul as the last of the preferences panels, all panels can be reloaded with the inspector open without triggering the issue.

Comment 11

[Julian Descottes [:jdescottes]]

Taking the bug for the time being.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8746070 [details] [diff] [review]
bug1260763.workaround.patch

Review of attachment 8746070 [details] [diff] [review]:
-----------------------------------------------------------------

This bug is rare enough (requires devtools open etc) that I think we should get an actual fix in here instead of changing the order of includes.

Comment 13

[Julian Descottes [:jdescottes]]

Comment on attachment 8746070 [details] [diff] [review]
bug1260763.workaround.patch

Thanks for having a look. Fine with skipping the workaround, I was not sure about the urgency.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

What happens if you add the event listener through JS? That would be a better workaround if it works, though it still doesn't solve the root problem.

Comment 15

[Julian Descottes [:jdescottes]]

Attachment: bug1260763.workaround.patch

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14)
> What happens if you add the event listener through JS? That would be a
> better workaround if it works, though it still doesn't solve the root
> problem.

It also works yes, the issue is only triggered if the listener is set in XUL.
Curious though, why is this better than changing the #include order?

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8746097 [details] [diff] [review]
bug1260763.workaround.patch

Review of attachment 8746097 [details] [diff] [review]:
-----------------------------------------------------------------

Changing the include order can affect the order that variables and functions are declared which in turn may have an unexpected effect on behavior. This is a safer work-around for the time being.

Comment 17

[Julian Descottes [:jdescottes]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> Comment on attachment 8746097 [details] [diff] [review]
> bug1260763.workaround.patch
> 
> Review of attachment 8746097 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Changing the include order can affect the order that variables and functions
> are declared which in turn may have an unexpected effect on behavior. This
> is a safer work-around for the time being.

Makes sense! 

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb6308f1dc24

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Looks we've at least got a workaround here.  Would be great to find the root cause, but I suspect it involves spelunking through XUL and event handling code to do so.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

It might be worth having Enn give this a poke.

Hey Enn - the STR in comment 4 - anything obvious in there jump out about where the bug probably is?

Comment 20

[Neil Deakin]

Attachment: Significantly reduced testcase

This testcase is very simplified and doesn't rely on opening in a preferences window.

I won't have time right now to investigate why, but I suspect that since the event listener code is running before the file has finished loading, it is causing JS/DOM wrappers to be created when you get the .target for elements that haven't been fully processed yet and the anonymous content is thus being placed incorrectly or something like that. I'm just guessing but it might be a path to start looking at.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3e2e4184eeff

Comment 22

[Julian Descottes [:jdescottes]]

Thanks for reducing the test case further!

I landed the workaround and moved the reduced test case to bug 1268790.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3e2e4184eeff

Comment 24

[Alice0775 White]

[Tracking Requested - why for this release]: UX broken

See Bug 1269834, In this case, It happens with combination of disabled autoupdate  and Inspector.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Regression from 46, tracking. 
Would this be good to uplift to at least aurora?  We are also heading into 47 beta 4 build next Monday.

Comment 27

[Julian Descottes [:jdescottes]]

Sorry I missed the needinfo! 

I landed the patch, but I can't really say if the issue is worth uplifting. 
Not sure the issue occurred anywhere but in the preference pane. Maybe addons developers could be impacted as well?

Forwarding the ni? to :jaws.

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

No, we should skip uplift on this as it was very rare to see and I usually prefer to get more bake-time, plus it was a not a new regression.

Comment 30

[:Gijs (he/him)]

Is there a bug to investigate and fix this rather than work around it? :-)

Comment 31

[Frankie]

Gijs: it's already fixed in FF49. See https://www.mozilla.org/en-US/firefox/channel/ and https://wiki.mozilla.org/RapidRelease/Calendar

Comment 32

[:Gijs (he/him)]

(In reply to Frankie from comment #31)
> Gijs: it's already fixed in FF49. See
> https://www.mozilla.org/en-US/firefox/channel/ and
> https://wiki.mozilla.org/RapidRelease/Calendar

Hi Frankie. I work for Mozilla, and I'm aware of how our release cycle works. But the patch from this bug is a workaround, not a fix for the underlying problem, hence my needinfo to track a "real" fix somewhere else. As it is, it is totally possible that we'll run into the same issue again in a different in-content page, which I'd like to avoid.

Comment 33

[Julian Descottes [:jdescottes]]

(In reply to :Gijs Kruitbosch from comment #30)
> Is there a bug to investigate and fix this rather than work around it? :-)

Yes, I logged Bug 1268790 to investigate the underlying problem.