619644 - Javascript confirm prompt breaks document.write iff closed with ESC (fine if cancel clicked)

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Stefan]

User-Agent:       
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b9pre) Gecko/20100101 Firefox/4.0b9pre

document.write after Pressing the ESC key in confirm dialog overwrites document.

Reproducible: Always

Steps to Reproduce:
1. open testcase https://bugzilla.mozilla.org/attachment.cgi?id=494685
2. press ESC
3.
Actual Results:  
"v = false"

Expected Results:  
"Lorem ipsum ... 
Do you agree?
v = false"

Built from http://hg.mozilla.org/mozilla-central/rev/8755d308796d

Comment 1

[Ed Morley [:emorley]]

Attachment: Reduced testcase

Comment 2

[Ed Morley [:emorley]]

Confirmed using Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101216 Firefox/4.0b9pre ID:20101216030331

Have reduced the testcase and so updated the STR to match.

New STR:
1) Open reduced testcase (attachment 498236 [details]) in new tab.
2) Press escape.
3) Observe content shown.
4) Repeat steps 1-3, except this time click cancel rather than pressing escape.

Expected:
- Same result regardless of escape or cancel - ie: Both "Original Content" and "Extra content" messages shown on the page.

Actual:
- If cancel clicked: see expected.
- If escape used, only the "Extra Content" message is shown, the "Original Content" message is overwritten.

Does not occur using 3.6.13, so is a regression in Fx4.

Marking as such and finding the regression window now.

Comment 3

[Ed Morley [:emorley]]

Last good nightly: 2010-11-19 
First bad nightly: 2010-11-20

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6478c1b83d60&tochange=baa6cc2f72e4

Suspect: Bug 59314 - Alerts should be content-modal, not window-modal

Provisionally marking as blocking that bug for visibility over there.

Adjusting title to clarify issue.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Very odd, not sure what's going on here. Think we need to block on figuring it out.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

We need to stop the ESC from propagating up to the handler in browser.js which aborts the document load.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

While fixing this I also discovered that going backwards when a tab-modal prompt is visible causes a compartment mismatch assertion.  I filed Bug 621297 for that.

Comment 9

[Stefan]

Your patch does not fix the "underlaying" problem in its totality:
STR

1. open reduced testcase -> spinner spins
2. press stop-button with mouse (stop loading page) -> spinner stops
3. press ESC key -> 
   a) original content vanishes
   b) spinner spins

Comment 10

[Stefan]

Ooops. Someone asked me to create a separate bug out of every symptom. So my previous Comment #9 hat to go to 619645 or do I have to file a new bug because the STR has changed??

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #10)
> Ooops. Someone asked me to create a separate bug out of every symptom. So my
> previous Comment #9 hat to go to 619645 or do I have to file a new bug because
> the STR has changed??

That's generally the right thing to do.  We can then dupe them once we're sure that they're the same underlying cause.

(In reply to comment #9)
> Your patch does not fix the "underlaying" problem in its totality:
> STR
> 
> 1. open reduced testcase -> spinner spins
> 2. press stop-button with mouse (stop loading page) -> spinner stops
> 3. press ESC key -> 
>    a) original content vanishes
>    b) spinner spins

So this is a slightly different issue.  Here we need to kill the prompt and all running scripts when stop is clicked.  Lets take this to a new bug (please post the number here though).

Thanks for your help with this!

Comment 12

[Stefan]

The new issue is in Bug 621308.

Comment 13

[Justin Dolske [:Dolske]]

Comment on attachment 499650 [details] [diff] [review]
Patch


>+// This is a little yucky, but it works
>+const expectedFinalDoc = 
>+"<head><\/head><body><p>Original content<\/p> ....

Add a comment here noting that this is the content of bug619644_inner.html.

Also, if you add a <head> and <body> to that file, does it then exactly match the string you're getting here? That would be nice just for clarity.

Comment 14

[Justin Dolske [:Dolske]]

(And thanks for the patch and omgtest!)

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/b2275b45a33d

(In reply to comment #13)
> Comment on attachment 499650 [details] [diff] [review]
> Patch
> 
> 
> >+// This is a little yucky, but it works
> >+const expectedFinalDoc = 
> >+"<head><\/head><body><p>Original content<\/p> ....
> 
> Add a comment here noting that this is the content of bug619644_inner.html.

Done

> Also, if you add a <head> and <body> to that file, does it then exactly match
> the string you're getting here? That would be nice just for clarity.

I believe so, but I pushed without changing that since I don't have time to compile a tree with this patch on this machine at the moment.  I'll investigate/fix soon.

(In reply to comment #14)
> (And thanks for the patch and omgtest!)

:-)

Comment 16

[Stefan]

New testcase: Kind of regression?

Comment 17

[Stefan]

Attachment: new testcase 2001-01-07

Comment 18

[Stefan]

STR:

1. open testcase
2. press button
3. press esc or press cancel or press OK

actual result:
keeps spinning.

expected result:
no spinning

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

That's by design.  document.write after the load is complete replaces the document.  It doesn't make much sense, but that's how it worked in the past so that's how it works now.

http://www.whatwg.org/specs/web-apps/current-work/multipage/apis-in-html-documents.html#document.write%28%29

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

And the still spinning part is a dupe of one of the other bugs (the "aborting the load has issues" one)

Comment 21

[Stefan]

@Kyle: Can't see any open bugs which cover this issue. Do you have a bug id?

Comment 22

[Ed Morley [:emorley]]

(In reply to comment #20)
> And the still spinning part is a dupe of one of the other bugs (the "aborting
> the load has issues" one)
Bug 621308 presumably?

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Oh, that happens with cancel and OK?  That's a diffferent issue, please file separately and CC me.

Comment 24

[Stefan]

Bug 623880 filed

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #13)
> Also, if you add a <head> and <body> to that file, does it then exactly match
> the string you're getting here? That would be nice just for clarity.

Done in http://hg.mozilla.org/mozilla-central/rev/8ba73a8038a9