1253891 - "prefStrBundle.getFormattedString is not a function" exceptions in in-content/advanced.js

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I was looking at a log for a M(BC5) run on Linux64 (e.g. <http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-4b1f80c98bed138964e10728195be517e8441800/try-linux64-debug/try_ubuntu64_vm-debug_test-mochitest-browser-chrome-5-bm124-tests1-linux64-build173.txt.gz> for a nice passing run) and saw four exceptions like this in the log:

20:27:41     INFO -  JavaScript error: chrome://browser/content/preferences/in-content/advanced.js, line 313: TypeError: prefStrBundle.getFormattedString is not a function

during the rowser/components/preferences/in-content/tests/browser_bug795764_cachedisabled.js test.

This seems pretty odd to me, and I haven't managed to reproduce it locally yet by actually going to the advanced pref pane...

Comment 1

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #0)
> I was looking at a log for a M(BC5) run on Linux64 (e.g.
> <http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> 4b1f80c98bed138964e10728195be517e8441800/try-linux64-debug/try_ubuntu64_vm-
> debug_test-mochitest-browser-chrome-5-bm124-tests1-linux64-build173.txt.gz>
> for a nice passing run) and saw four exceptions like this in the log:
> 
> 20:27:41     INFO -  JavaScript error:
> chrome://browser/content/preferences/in-content/advanced.js, line 313:
> TypeError: prefStrBundle.getFormattedString is not a function
> 
> during the
> rowser/components/preferences/in-content/tests/
> browser_bug795764_cachedisabled.js test.
> 
> This seems pretty odd to me, and I haven't managed to reproduce it locally
> yet by actually going to the advanced pref pane...

The test in question opens the preferences, then the advanced pane, then does some sync checks on which elements are visible, and then closes the tab.

The advanced prefs do some async fetches of storage/cache information and then display the results. If those fetches come in when the tab has already closed, the document and its nodes might be alive (so you can getElementById the string bundle and get an element back) but the XBL binding will be dead, and so getFormattedString is gone.

There's at least a few options here....
1) try to see if we can remove the observers (not sure we can in all of the cases in advanced.js as they're going through the storage and cache service) when the page unloads
2) null-check getFormattedString and bail out of the observers;
3) do nothing because this only happens in automated tests and has no real ill effects if you managed to reproduce it in normal browser usage

I don't really feel strongly either way. Jared?

Comment 2

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

Option #3 is my choice here.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So to be clear, this is blocking me landing some changes to xpconnect to improve error reporting.  In particular, the changes cause the exception in this case to actually be associated with the window the code is in, instead of the junk scope as now.  That keeps the window object alive for a little bit after the test (I'm still hunting down exactly what does that).  And then the leaked window detection in browser-test.js fails the test suite.

Comment 4

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

Sorry, I didn't realize this was blocking you. We can do option #2 then as it is simple and should prevent the error, as well we should add a comment describing what is happening here.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Awesome, thanks.  I'm still trying to figure out why the window stays alive longer, by the way; so far I haven't found something that would actually cause that...

Comment 6

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #5)
> Awesome, thanks.  I'm still trying to figure out why the window stays alive
> longer, by the way; so far I haven't found something that would actually
> cause that...

Any chance that's actually related to these observers (could just comment the whole bunch of code here out to see) ?

Comment 7

[Boris Zbarsky [:bzbarsky]]

If I just try/catch around the calls to getFormattedString in these observers, my oranges go away.  So it's definitely related to the exceptions thrown by these observers...  I'm not sure that's what you're asking though?

Comment 8

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

Attachment: MozReview Request: Bug 1253891 - "prefStrBundle.getFormattedString is not a function" exceptions in in-content/advanced.js. r?gijs

Guards were added to both async calls as it is likely that either of them could fire after the tab has been closed.

Review commit: https://reviewboard.mozilla.org/r/38499/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38499/

Comment 9

[:Gijs (he/him)]

Comment on attachment 8727531 [details]
MozReview Request: Bug 1253891 - "prefStrBundle.getFormattedString is not a function" exceptions in in-content/advanced.js. r?gijs

https://reviewboard.mozilla.org/r/38499/#review35095

r=me but the check for the element itself is not currently strictly necessary AIUI.

Comment 10

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

Thanks. I added the check for the element because it seemed like extra added protection, but I'll remove it.

Comment 11

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

Attachment: Patch for check-in

Comment 12

[Pulsebot]

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

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c4f5d75e11f2