Closed Bug 637644 Opened 13 years ago Closed 13 years ago

adding elements through javascript to Popup windows does not work.

Categories

(Core :: DOM: Core & HTML, defect, P1)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla5
Tracking Flags:
Tracking Status
blocking2.0 --- .x+

People

(Reporter: fur_eyal, Assigned: bzbarsky)

References

Details

Attachments

(4 files, 1 obsolete file)

a short html page that demonstrates the problem.
356 bytes, text/html
Details
Start layout on the new document in CreateAboutBlankContentViewer, since if we don't do it here no one ever will.
5.71 KB, patch
jst
: review+
Details | Diff | Splinter Review
Screenshot of the opened window which is transparent on Mac
262.30 KB, image/png
Details
Start layout on the new document in CreateAboutBlankContentViewer, since if we don't do it here no one ever will. try: -b do -p all -u all -t nochrome,chrome,dirty,cold
5.84 KB, patch
jst
: review+
beltzner
: approval2.0-
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12) Gecko/20100101 Firefox/4.0b12
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12) Gecko/20100101 Firefox/4.0b12

it is possible in all other browsers (including. firefox 3.6.13) to create a popup through 
var popup = window.open("", "",  "height=550, width=550,left=300, top=300");

and then work with it, e.g.
	var doc = popup.document;
	doc.body.appendChild(doc.createTextNode("I'm here"));

this adds a textnode to the popup in ff 3.6, ie, chrome, safari, opera, konqueror and more, but fails on ff 4.0b12

Reproducible: Always

Steps to Reproduce:
1. create a file whose content is the "additional information" attached.
2.open it with FF 4.0b12
3.press the "press me" button
Actual Results:  
an empty popup opens

Expected Results:  
a popup opens, containing the text "I'm here"

<html>
<head>
<script type="text/javascript">

function openPopup() {
	var popup = window.open("", "", "resizable=1,height=550, width=550,left=300, top=300");
	var doc = popup.document;
	doc.body.appendChild(doc.createTextNode("I'm here"));
}
</script>
<body>
<input type = "button" onclick="javascript:openPopup();" value = "Press me" />
</body>
</html>
open attached page. with 3.6.13, pressing the "press me" button opens a popup with some content (a single textNode: "I'm here"). with ff 4.0b12 the opened popup is empty.
Confirmed on Build identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b13pre) Gecko/20110301 Firefox/4.0b13pre.

Firefox 4 Beta 6 works fine but it is broken in Firefox 4 Beta 7.
Firefox 3.6.13 and Google Chrome 9 work fine.
Assignee: nobody → general
Status: UNCONFIRMED → NEW
Component: General → JavaScript Engine
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
This has nothing to do with JS, as far as I can tell.

More precisely, the node is in the DOM.  It's just not getting a CSS box created for it, which has nothing to do with the JS engine...
In fact, looks a lot like bug 598895, but that one is NOT a regression.

Henri, any idea what might have changed around b7 here?  You've been in that code most recently....
Assignee: general → nobody
Component: JavaScript Engine → DOM: Core & HTML
Depends on: 598895
QA Contact: general → general
m-c regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=178f26e21cfc&tochange=ad0a0be8be74

This looks like it might be the new wrapper stuff.  Blake?
This might be worth blocking on, since it's a pretty clear web compat regression, though the fact that this is the first time it was noticed in the last 4 months argues against that...
blocking2.0: --- → ?
This is a regression from bug 603152.

Before that patch, we'd load the about:blank, and the parser would StartLayout on it.

With the patch we then proceed to blow away the old about:blank using CreateAboutBlankContentViewer and then nothing calls StartLayout on the new thing.
Blocks: 603152
I should have a safe fix for this in a few mins (which should also fix bug 598895.
Assignee: nobody → bzbarsky
Blocks: 598895
No longer depends on: 598895
Attachment #516319 - Flags: review?(jst) → review+
Comment on attachment 516319 [details] [diff] [review]
Start layout on the new document in CreateAboutBlankContentViewer, since if we don't do it here no one ever will.

Requesting approval.  This is a very safe patch that fixes a noticeable regression from 3.6.
Attachment #516319 - Flags: approval2.0?
Whiteboard: [need approval]
blocking2.0: ? → final+
Whiteboard: [need approval] → [hardblocker]
Attachment #516319 - Flags: approval2.0?
Whiteboard: [hardblocker] → [need landing][hardblocker]
Pushed http://hg.mozilla.org/mozilla-central/rev/729cbdcae605
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Priority: -- → P1
Resolution: --- → FIXED
Whiteboard: [need landing][hardblocker] → [hardblocker]
Target Milestone: --- → mozilla2.0
The hg link in comment 11 gives me an error.  I think this is the right one:

http://hg.mozilla.org/mozilla-central/rev/4b7c9efbf269
These perma-oranges have also happened on Windows too.

Still waiting to see if the backout fixed things.
Possible Txul regression here too:

Regression :( Txul increase 7.55% on Linux x64 Firefox
------------------------------------------------------
    Previous: avg 96.902 stddev 0.805 of 30 runs up to revision 7bc88a8f9095
    New     : avg 104.221 stddev 0.452 of 5 runs since revision c1aecf6ba5e7
    Change  : +7.319 (7.55% / z=9.088)
    Graph   : http://mzl.la/gtJ9TH

Changeset range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7bc88a8f9095&tochange=c1aecf6ba5e7

Changesets:
  * http://hg.mozilla.org/mozilla-central/rev/4b7c9efbf269
    : Boris Zbarsky <bzbarsky@mit.edu> - Bug 637644.  Start layout on the new document in CreateAboutBlankContentViewer, since if we don't do it here no one ever will.  r=jst, a=blocker
    : http://bugzilla.mozilla.org/show_bug.cgi?id=637644

  * http://hg.mozilla.org/mozilla-central/rev/38028bf13fb2
    : Marco Bonardo <mbonardo@mozilla.com> - Bug 637957 - Followup, add proper QI. rs=mossop a=blocker-orange-fix
    : http://bugzilla.mozilla.org/show_bug.cgi?id=637957

  * http://hg.mozilla.org/mozilla-central/rev/c1aecf6ba5e7
    : Jeff Walden <jwalden@mit.edu> - Bug 637385 - Don't try to trace through a bindname in strict mode eval code.  r=dvander, a=dmandelin
    : http://bugzilla.mozilla.org/show_bug.cgi?id=637385
I'd like to re-visit blocking+ on this. Boris made a really good point in comment #6 "it's a pretty clear web compat regression, though the fact that this is the first time it was noticed in the last 4 months argues against that..."

Given that this isn't as trivial as it looked and that we don't have any reports of this breaking any top sites, I'd like us to consider minusing it.
I can't explain why the window opened during the failing test is tranparent!
Removing this from the hardblocker list - hasn't really been seen a lot in the past months, I agree with Asa.
blocking2.0: final+ → .x+
bc, do you think you could spider for a pattern like in this bug and see if it
shows up elsewhere?
Yeah, we should definitely postpone this.

I'm going to need help with this a11y test.  It seems to be assuming things about the plethora of about:blanks in the about:blank part of this test (there are at least 3 separate documents there) that are just not justified...
Whiteboard: [hardblocker]
(In reply to comment #22)
> bc, do you think you could spider for a pattern like in this bug and see if it
> shows up elsewhere?

On first glance I'm not sure how to detect that the DOM methods were used on a popup window by parsing the javascript. Perhaps if I could insert some javascript that would be invoked for appendChild, removeChild, replaceChild I could detect it. Does anyone have any pointers on how to do that?

I have 2 free linux boxes I can devote to this which could start scanning the alexa list and could also put this on the crash automation, but to get reasonable coverage (whatever that means) could take more time that we have before rc.
OK, I'm going to work around the a11y test by moving the InitialReflow call into nsGlobalWindow.  That makes that test pass, while not breaking the regression tests for this bug...
Target Milestone: mozilla2.0 → ---
Fix per comment 25.  This passes the a11y test locally on Linux and passes the two new tests.  I've pushed it to try this time, instead of relying on local test runs.
(In reply to comment #23)
Given a11y test passes as per comment 26, I've filed bug 638313 to properly improve the a11y test.
Attachment #516468 - Flags: review?(jst)
Attachment #516468 - Flags: review?(jst) → review+
Oh, and Ehsan's transparent thing has nothing to do with this patch; he checked by local backout.
(In reply to comment #24)
> (In reply to comment #22)
> > bc, do you think you could spider for a pattern like in this bug and see if it
> > shows up elsewhere?
> 
> On first glance I'm not sure how to detect that the DOM methods were used on a
> popup window by parsing the javascript. Perhaps if I could insert some
> javascript that would be invoked for appendChild, removeChild, replaceChild I
> could detect it. Does anyone have any pointers on how to do that?

That's tricky.  Example:

function foo(win) {
  var doc = win.document;
  doc.accessTheDOM();
}

foo(window);
foo(open("mypage.html"));
(In reply to comment #28)
> Oh, and Ehsan's transparent thing has nothing to do with this patch; he checked
> by local backout.

That might be something for davidb to investigate too...
Whiteboard: [need review]
Maybe I could watch when windows are opened and attach dom mutation listeners.
Should we anyway take the new patch to FF4? The regression is quite bad, IMO.
For what it's worth, the new patch passes tests on try server.  Performance-wise it seems to not show Txul or Ts regressions; I'm still running more tests to double-check that.
please give MarcoZ a try build for QA just in case (unfortunately I'm away today)
Oh, and there seems to be no obvious perf regression there.
Well, I do not see this newly inserted content at all, which might be another bug, but I don't see a difference between the try-server build, the current nightly, and the hourly build containing bug 638106, so I don't think this makes a difference to us. Why we don't see that newly inserted document content I don't know. We do have an internal frame accessible but no document accessible in EITHER of these builds.
Note that the about:blank document is only there for a few milliseconds at the most; then it gets blown away by the thing that's really loading in that window.
Blocks: 638465
I'm assuming that I will land that fix after fx4 ships.
Whiteboard: [need review] → [need gk2 ship]
Comment on attachment 516468 [details] [diff] [review]
Start layout on the new document in CreateAboutBlankContentViewer, since if we don't do it here no one ever will.    try: -b do -p all -u all -t nochrome,chrome,dirty,cold

To me the patch (the latter one) doesn't look too regression risky, yet
it should fix quite a bad regression.
Attachment #516468 - Flags: approval2.0?
Comment on attachment 516468 [details] [diff] [review]
Start layout on the new document in CreateAboutBlankContentViewer, since if we don't do it here no one ever will.    try: -b do -p all -u all -t nochrome,chrome,dirty,cold

If we do an RC2, we can reconsider this, but I don't think we should take it for RC1
Attachment #516468 - Flags: approval2.0? → approval2.0-
Pushed http://hg.mozilla.org/mozilla-central/rev/906db7ecd063
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Whiteboard: [need gk2 ship]
Target Milestone: --- → mozilla2.2
Blocks: 643233
as per comment 42 this issue is fixed and pushed into code tree (and stauts is "resoved/fixed), but all this happened well before 4.0.1, and yet 4.0.1 still have the problem. 
is the fix really in the code? if it is it seems that it didn't do its work. should i reopen?

or if this fix is not actually in 4.0.1, then the question is: any prognosis when this fix will get into distributed code?
my users are somewhat impatient for not being able to use ff....

peace.
This fix was checked in to the development trunk.  Firefox 4.0.1 was a security-fixes-only release from the Firefox 4 branch.

The target milestone of this bug says when this fix will be in final: Firefox 5.  You can test the Aurora builds right now; they have the fix.

Firefox 5 final will ship around June 22.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: