In utilityOverlay.js, openNewTabWith() has the block: // As in openNewWindowWith(), we want to pass the charset of the // current document over to a new tab. var originCharset = aDocument && aDocument.characterSet; if (!originCharset && document.documentElement.getAttribute("windowtype") == "navigator:browser") originCharset = window.content.document.characterSet; window.content.document may be null in this case and this causes the test browser_utilityOverlay.js to fail.

Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".

Mark, Did you actually see the test fail, or did you deduce it from reading the code? My local copy seems to pass.

I actually saw browser/base/content/test/general/browser_utilityOverlay.js fail - but can no longer reproduce it. So I guess this should be closed as WFM and a followup opened to re-enable browser/base/content/test/general/browser_utilityOverlay.js in e10s (or maybe just morph this bug into the re-enable of the test)

The problem in comment 0 still exists, whether the test is failing or not. Seems like that should be using gBrowser.selectedBrowser.characterSet (taking advantage of the browser.characterSet property bug 691613 added). (We should probably also file a bug on mirroring gBrowser.selectedBrowser.characterSet onto gBrowser.characterSet, like we do for other browser.xml-defined properties, see http://hg.mozilla.org/mozilla-central/annotate/c962bde5ac0b/browser/base/content/tabbrowser.xml#l2796)

Gavin & Mark, Bill & I planned to patch the code, using the characterSet property from remote-browser.xml. I needed to know whether or not I had more work to do on the test itself before sending the patch around.

Bill, look good from your prespective?

Comment on attachment 8410710 [details] [diff] [review] e10s-fail-utilitiesOverlay Yup, seems good to me.

Comment on attachment 8410710 [details] [diff] [review] e10s-fail-utilitiesOverlay > if (aDocument) > urlSecurityCheck(aURL, aDocument.nodePrincipal); > var originCharset = aDocument && aDocument.characterSet; Is this stuff sane for e10s? > if (!originCharset && > document.documentElement.getAttribute("windowtype") == "navigator:browser") >- originCharset = window.content.document.characterSet; >+ originCharset = window.gBrowser.selectedBrowser.characterSet; Please remove "window.".

(In reply to Dão Gottwald [:dao] from comment #8) > Comment on attachment 8410710 [details] [diff] [review] > e10s-fail-utilitiesOverlay > > > if (aDocument) > > urlSecurityCheck(aURL, aDocument.nodePrincipal); > > > var originCharset = aDocument && aDocument.characterSet; > > Is this stuff sane for e10s? If aDocument is a content document, then no. However, all the callers of openNewTabWith in mozilla-central pass null for the document. Ideally we would just remove this argument. However, I searched AMO and there are tons of callers, and some of them pass something for this parameter (although it usually isn't a document, strangely). So I think we should just fix the charset thing for now. Also, Ally, could you please fix openNewWindowWith in the same way?

(In reply to Bill McCloskey (:billm) from comment #9) > (In reply to Dão Gottwald [:dao] from comment #8) > > Comment on attachment 8410710 [details] [diff] [review] > > e10s-fail-utilitiesOverlay > > > > > if (aDocument) > > > urlSecurityCheck(aURL, aDocument.nodePrincipal); > > > > > var originCharset = aDocument && aDocument.characterSet; > > > > Is this stuff sane for e10s? > > If aDocument is a content document, then no. However, all the callers of > openNewTabWith in mozilla-central pass null for the document. Ideally we > would just remove this argument. However, I searched AMO and there are tons > of callers, and some of them pass something for this parameter (although it > usually isn't a document, strangely). Sounds like these consumers are already broken and we wouldn't break them any more by removing support for that argument. Note that even if they behaved correctly, e10s would break them anyway, since aDocument only makes sense as a content document.

> Also, Ally, could you please fix openNewWindowWith in the same way? Of course, that's why I passed over a f?. Wanted to make sure everyone is happy with one before I patched hte other.

I talked to Gavin. He concurs with Dao with respect to aDocument, that it should be an ignored parameter but should not be removed from the function signature. I have updated the patch & updated/added comments accordingly. However, this has introduced a test failure. TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base content/test/general/browser_utilityOverlay.js | leaked window property: origin Charset current wip attached for those playing along at home.

it helps to write your patches to disk before running tests or uploading. Tests pass.

Comment on attachment 8411451 [details] [diff] [review] e10s_overlay Review of attachment 8411451 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/utilityOverlay.js @@ +605,5 @@ > * @param aURL > * The URL to open (as a string). > * @param aDocument > + * Note this parameter is now ignored. There is no security check & no > + * referrer header derived from aDocument (null case). nit: whitespace at the end @@ +654,3 @@ > } > > + nit: extra line added

https://hg.mozilla.org/integration/fx-team/rev/0e0d3955de46 Friendly reminder that your commit message should be summarizing what the patch is doing, not restating the problem it's fixing :) https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

I didn't know. I thought it was always the bug title, end of story. Thanks!