Some in-content preferences panes are not showing

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mconley, Assigned: jdescottes)

Tracking

({regression, reproducible})

46 Branch
Firefox 49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 wontfix, firefox47- wontfix, firefox48- wontfix, firefox49+ fixed, firefox-esr38 unaffected, firefox-esr45 unaffected)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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.
jaws, have you heard of anything like this before?
Flags: needinfo?(jaws)
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?
Flags: needinfo?(jaws) → needinfo?(rfeeley)
Summary: Some in-content preferences panes are now showing → Some in-content preferences panes are not showing
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.
Flags: needinfo?(rfeeley)
Reporter

Comment 4

3 years ago
STR
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.
Keywords: reproducible
It looks as if the Inspector is somehow interfering with the construction of the pref panes.

jryans, any ideas what's going on?
Flags: needinfo?(jryans)
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
Flags: needinfo?(jryans) → needinfo?(jdescottes)
Blocks: 1157469
Keywords: regression
Assignee

Comment 7

3 years ago
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.
Assignee

Comment 8

3 years ago
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).
Flags: needinfo?(jdescottes)
Assignee

Comment 9

3 years ago
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?
Flags: needinfo?(mconley)
Flags: needinfo?(jryans)
Posted patch bug1260763.workaround.patch (obsolete) — Splinter Review
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.
Attachment #8746070 - Flags: review?(jaws)
Taking the bug for the time being.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Keywords: leave-open
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.
Attachment #8746070 - Flags: review?(jaws) → review-
Assignee

Updated

3 years ago
Keywords: leave-open
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.
Attachment #8746070 - Attachment is obsolete: true
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.
(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?
Attachment #8746097 - Flags: review?(jaws)
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.
Attachment #8746097 - Flags: review?(jaws) → review+
(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
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.
Flags: needinfo?(jryans)
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?
Flags: needinfo?(mconley) → needinfo?(enndeakin)

Comment 20

3 years ago
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.
Flags: needinfo?(enndeakin)
Thanks for reducing the test case further!

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

Updated

3 years ago
See Also: → 1268790

Comment 23

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3e2e4184eeff
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

Comment 24

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

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

Updated

3 years ago
Duplicate of this bug: 1269834
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.
Flags: needinfo?(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.
Flags: needinfo?(jdescottes) → needinfo?(jaws)
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.
Flags: needinfo?(jaws)
Assignee

Updated

3 years ago
Duplicate of this bug: 1256703

Comment 30

3 years ago
Is there a bug to investigate and fix this rather than work around it? :-)
Flags: needinfo?(jdescottes)

Comment 31

3 years ago
Gijs: it's already fixed in FF49. See https://www.mozilla.org/en-US/firefox/channel/ and https://wiki.mozilla.org/RapidRelease/Calendar
Flags: needinfo?(jdescottes)

Comment 32

3 years ago
(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.
Flags: needinfo?(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.
Flags: needinfo?(jdescottes)
Version: unspecified → 46 Branch
You need to log in before you can comment on or make changes to this bug.