Closed Bug 1180525 Opened 9 years ago Closed 9 years ago

A function from a window which got blown away by document.open is not invoked when used as an event listener

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Version:
37 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: don-mozilla, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(2 files)

testcase un-commented
(deleted), text/html
Details
Run callbacks whose window is no longer current as long as its document is the active document in the browsing context
(deleted), patch
bholley
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150416103922

Steps to reproduce:

I start with the following html file:
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"> 
<html> 
  <head> 
<script type="text/javascript"> 
function fff(){alert("fff");} 
window.onload=function(e){ 
// document.write('<div id="x"></div>\n'); 
 document.getElementById("x").innerHTML="<input type='submit' value='xxx' id='xx\
x'>"; 
 document.getElementById("xxx").addEventListener("click", fff, false); 
} 
</script> 
    <title>test</title> 
  </head> 
  <body> 
<div id="x"></div> 
  </body> 
</html>


Actual results:

This shows a button, when I click on it I get an alert.
This works everywhere I've tried it, inc. FF 22.0 and FF 37.0.2
However, if I uncomment the document write, it no longer works in FF 37.0.2
It still works everywhere else I've tried (not that many places, I admit)


Expected results:

I expected that what worked in other versions would continue to work in 37.0.2
(this is on linux, fedora 21, btw)
Blocks: 862627
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Events
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
I don't understand "regressed by".  You agree this is a bug?  Is there a way to fix it in my
current browser?  More likely I have to wait for an update?
$ yum list installed firefox
reports
firefox.x86_64                      37.0.2-1.fc21                       @updates
It means that Alice has confirmed that the behaviour you see is still present in the latest Firefox, and found at what point it was introduced. The next step is that the person who introduced the behaviour investigates whether this is the expected behaviour, and if it isn't, corrects it.
"regressed by" means that this used to work, and it stopped working when bug 862627 was fixed.

What's going on here is that the implicit document.open() from the post-onload document.write() replaces the old Window with a new one.  And CallbackObject has a check to make sure we're not invoking a callback that comes from a non-current Window.

We _could_ probably change that check to be a HasActiveDocument() check instead of what is effectively an IsCurrentInnerWindow() check.  That seems ok to me: it wouldn't affect the entry document situation (which is what the current check is documented as addressing), since it's in fact the same document.

Bobby, thoughts?
Flags: needinfo?(bobbyholley)
Summary: addeventlistener after document write → A function from a window which got blown away by document.open is not invoked when used as an event listener
(In reply to Boris Zbarsky [:bz] from comment #4)
> "regressed by" means that this used to work, and it stopped working when bug
> 862627 was fixed.
> 
> What's going on here is that the implicit document.open() from the
> post-onload document.write() replaces the old Window with a new one.  And
> CallbackObject has a check to make sure we're not invoking a callback that
> comes from a non-current Window.
> 
> We _could_ probably change that check to be a HasActiveDocument() check
> instead of what is effectively an IsCurrentInnerWindow() check.  That seems
> ok to me: it wouldn't affect the entry document situation (which is what the
> current check is documented as addressing), since it's in fact the same
> document.
> 
> Bobby, thoughts?

That seems good to me. My primary concern on stuff like this is aligning with other browsers. I checked and Blink+WebKit seem to run the listener, so I think we should too.
Flags: needinfo?(bobbyholley)
Blink+webkit run listeners in totally unloaded windows too.  See <http://web.mit.edu/bzbarsky//www/testcases/events/event-listener-from-unloaded-document-1.html>.

I tried IE with the attached testcase, but it throws an exception because the getElementById("x") call returns null.

Anyway, I guess I'll do the HasActiveDocument() thing for now, until a spec actually defines this stuff.
The distinction only matters when document.open() makes a different
window current without changing the active document.
Attachment #8630329 - Flags: review?(bobbyholley)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8630329 - Flags: review?(bobbyholley) → review+
OK, so we have a few try failures:

1)  dom/base/test/test_bug372964-2.html | Shouldn't have handled an event because the context has changed - got 2, expected 1.  This test is using open()/close() to change out the window and then fire an event at an XHR instance.  That's precisely what this patch is trying to allow.  Talked to Olli and will change this test to actually navigate the browsing context.

2)  Unexpected passes in WPT html/dom/dynamic-markup-insertion/opening-the-input-stream/016.html.  Looking at the structure of this test, I agree it should pass after this change and that it doing so is OK.

3)  Unexpected fail and unexpected OK on WPT old-tests/submission/Opera/script_scheduling/094.html due to reaching the assert_unreached.  I know why we're doing that now (because the setTimeout fires, when it didn't use to), but why exactly does this test expect the setTimeout to not fire or if it does for the assert_unreached to not fail the test?  James, is this test using the test harness properly?

4)  Unexpected pass on WPT old-tests/submission/Opera/script_scheduling/101.html -- this behavior change is expected.
Flags: needinfo?(james)
Per irc discussion, the test is making bogus assumptions; it doesn't consider the fact that the timeout is created after the new document is created, or the fact that the document.open() call overwrites the original test so we can't collect the result.
Flags: needinfo?(james)
https://hg.mozilla.org/mozilla-central/rev/e3a5dbe2d697
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: