Closed
Bug 1260763
Opened 9 years ago
Closed 9 years ago
Some in-content preferences panes are not showing
Categories
(Firefox :: Settings UI, defect)
Tracking
()
RESOLVED
FIXED
Firefox 49
Tracking | Status | |
---|---|---|
firefox46 | --- | wontfix |
firefox47 | - | wontfix |
firefox48 | - | wontfix |
firefox49 | + | fixed |
firefox-esr38 | --- | unaffected |
firefox-esr45 | --- | unaffected |
People
(Reporter: mconley, Assigned: jdescottes)
References
Details
(Keywords: regression, reproducible)
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
(deleted),
application/vnd.mozilla.xul+xml
|
Details |
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.
Reporter | ||
Comment 1•9 years ago
|
||
jaws, have you heard of anything like this before?
Flags: needinfo?(jaws)
Comment 2•9 years ago
|
||
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
Comment 3•9 years ago
|
||
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•9 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
Reporter | ||
Comment 5•9 years ago
|
||
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)
Comment 6•9 years ago
|
||
str |
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•9 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•9 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•9 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)
Assignee | ||
Comment 10•9 years ago
|
||
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)
Assignee | ||
Comment 11•9 years ago
|
||
Taking the bug for the time being.
Comment 12•9 years ago
|
||
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•9 years ago
|
Keywords: leave-open
Assignee | ||
Comment 13•9 years ago
|
||
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
Comment 14•9 years ago
|
||
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.
Assignee | ||
Comment 15•9 years ago
|
||
(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 16•9 years ago
|
||
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+
Assignee | ||
Comment 17•9 years ago
|
||
(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)
Reporter | ||
Comment 19•9 years ago
|
||
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•9 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)
Assignee | ||
Comment 22•9 years ago
|
||
Thanks for reducing the test case further!
I landed the workaround and moved the reduced test case to bug 1268790.
Assignee | ||
Updated•9 years ago
|
Comment 23•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment 24•9 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.
status-firefox46:
--- → wontfix
status-firefox47:
--- → affected
status-firefox48:
--- → affected
status-firefox-esr38:
--- → unaffected
status-firefox-esr45:
--- → unaffected
tracking-firefox47:
--- → ?
tracking-firefox48:
--- → ?
tracking-firefox49:
--- → ?
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.
Assignee | ||
Comment 27•9 years ago
|
||
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)
Comment 28•9 years ago
|
||
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)
Comment 30•8 years ago
|
||
Is there a bug to investigate and fix this rather than work around it? :-)
Flags: needinfo?(jdescottes)
Comment 31•8 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•8 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)
Assignee | ||
Comment 33•8 years ago
|
||
(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)
Updated•8 years ago
|
Version: unspecified → 46 Branch
You need to log in
before you can comment on or make changes to this bug.
Description
•