Closed
Bug 921947
Opened 11 years ago
Closed 11 years ago
openNewTabWith/openNewWindowWith reference content document which may be null.
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 31
Tracking | Status | |
---|---|---|
e10s | + | --- |
People
(Reporter: markh, Assigned: ally)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
5.74 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → ally
Assignee | ||
Updated•11 years ago
|
Assignee: ally → nobody
Comment 1•11 years ago
|
||
Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".
tracking-e10s:
--- → +
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → ally
Updated•11 years ago
|
Blocks: old-e10s-m2
Assignee | ||
Comment 2•11 years ago
|
||
Mark,
Did you actually see the test fail, or did you deduce it from reading the code? My local copy seems to pass.
Flags: needinfo?(mhammond)
Assignee | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 3•11 years ago
|
||
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)
Flags: needinfo?(mhammond)
Comment 4•11 years ago
|
||
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)
Assignee | ||
Comment 5•11 years ago
|
||
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.
Assignee | ||
Comment 6•11 years ago
|
||
Bill, look good from your prespective?
Attachment #8410710 -
Flags: feedback?(wmccloskey)
Comment on attachment 8410710 [details] [diff] [review]
e10s-fail-utilitiesOverlay
Yup, seems good to me.
Attachment #8410710 -
Flags: review?(felipc)
Attachment #8410710 -
Flags: feedback?(wmccloskey)
Attachment #8410710 -
Flags: feedback+
Comment 8•11 years ago
|
||
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?
Comment 10•11 years ago
|
||
(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.
Assignee | ||
Comment 11•11 years ago
|
||
> 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.
Assignee | ||
Updated•11 years ago
|
Attachment #8410710 -
Flags: review?(felipc)
Assignee | ||
Comment 12•11 years ago
|
||
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.
Attachment #8410710 -
Attachment is obsolete: true
Assignee | ||
Comment 13•11 years ago
|
||
it helps to write your patches to disk before running tests or uploading. Tests pass.
Attachment #8411444 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8411451 -
Flags: review?(felipc)
Comment 14•11 years ago
|
||
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
Attachment #8411451 -
Flags: review?(felipc) → review+
Assignee | ||
Comment 15•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Updated•11 years ago
|
Attachment #8411451 -
Attachment is obsolete: true
Comment 16•11 years ago
|
||
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
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Comment 17•11 years ago
|
||
I didn't know. I thought it was always the bug title, end of story. Thanks!
Comment 18•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
You need to log in
before you can comment on or make changes to this bug.
Description
•