Closed Bug 921947 Opened 6 years ago Closed 6 years ago

openNewTabWith/openNewWindowWith reference content document which may be null.

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

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)

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: nobody → ally
Assignee: ally → nobody
Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".
tracking-e10s: --- → +
Assignee: nobody → ally
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)
Status: NEW → ASSIGNED
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)
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.
Attached patch e10s-fail-utilitiesOverlay (obsolete) — Splinter Review
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 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.
Attachment #8410710 - Flags: review?(felipc)
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
Attached patch e10s_overlay (obsolete) — Splinter Review
it helps to write your patches to disk before running tests or uploading. Tests pass.
Attachment #8411444 - Attachment is obsolete: true
Attachment #8411451 - Flags: review?(felipc)
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+
Keywords: checkin-needed
Attachment #8411451 - Attachment is obsolete: true
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]
I didn't know. I thought it was always the bug title, end of story. Thanks!
https://hg.mozilla.org/mozilla-central/rev/0e0d3955de46
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.