459443 - [FIX]Scripts are disabled when navigating away from designMode document

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[David Bloom]

User-Agent:       Opera/9.60 (X11; Linux x86_64; U; en) Presto/2.1.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081010 Minefield/3.1b2pre

See attached testcase. It fails in Firefox 3.0.3 and in the 20081010 nightly.

In certain situations, Firefox will not run scripts when navigating away from a designMode document. This has regressed since Firefox 2.

Reproducible: Always

Steps to Reproduce:
(see testcase)
Actual Results:  
fail

Expected Results:  
pass

Comment 1

[David Bloom]

Attachment: Testcase

Comment 2

[Ria Klaassen (not reading all bugmail)]

This regressed between 2008042315 and 2008042317 so it is probably a result of  Bug 386782.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Perhaps some additional DetachEditorFromWindow calls are needed in nsDocShell?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

On Linux I also see the regression as between 2008-04-23-04-trunk and 2008-04-24-04-trunk.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Peter, Chris, can one of you take this? It's P1.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

It looks like we're not firing the page hide notification, which detaches the editor, and in doing so re-enables script. nsDocShell::DetachEditorFromWindow() doesn't detach when nsDocShell::mOSHE is null, but when mOSHE is null we sometimes have an mEditorData, so we should detach regardless of whether mOSHE is null or not. I'm not sure if mOSHE is supposed to be null. A mochitest based on the testcase is really flaky, so there's more to it than that, I'll keep investigating tomorrow...

We're also getting this assertion failure:

###!!! ASSERTION: Adding child where we already have a child?  This will likely misbehave: 'Error', file c:/work/src/orange/docshell/shistory/src/nsSHEntry.cpp, line 598

I'm not sure what that means, sounds bad.

Comment 7

[Martijn Wargers (dead)]

(In reply to comment #6)
> We're also getting this assertion failure:
> 
> ###!!! ASSERTION: Adding child where we already have a child?  This will likely
> misbehave: 'Error', file
> c:/work/src/orange/docshell/shistory/src/nsSHEntry.cpp, line 598
> 
> I'm not sure what that means, sounds bad.

There are various bugs filed for that assertion.

Comment 8

[Boris Zbarsky [:bzbarsky]]

That assertion will happen when reloading pages with iframes or performing various kinds of DOM manipulation on iframes... 

mOSHE being null might in fact be right here, or at least "on purpose".  In nsDocShell::OnStateChange in the wyciwyg case we don't set mOSHE to mLSHE unless mOSHE is non-null.  Might be worth doing some CVS archeology to find out why, especially since we _do_ add the write to session history.  Are we expecting someone else to do the set later?

Do we know why the null-check in DetachEditorFromWindow got added?  Again, CVS archeology.

Not sure what to make of the "not firing the page hide notification" part is about.  I assume we are in fact firing it and just ending up hitting the DetachEditorFromWindow() null-check?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Chris is away for a few weeks. Boris, do you think you might be able to take this?

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK. So as far as setting mOSHE in OnStateChange, it looks like that was the initial checkin of the code for bug 35340.  I'm not quite sure why that check is there, but we're certainly getting to that point with a null mOSHE, so not setting it.  I have no idea why that null-check is there, honestly.  Nothing ever ends up setting mOSHE to that mLSHE after that point, as far as I can tell.

As far as the null-check in DetachEditorFromWindow, I think that was just preserving the existing null-check when bug 430624 got checked in.  That patch is by peterv, r=cpearce.  The existing null-check was added in bug 386782, and that patch is by cpearce, r=peterv....

Heck, why is cpearce asking me about this code?  ;)

That said, I think both null-checks should probably go away.  Here's hoping Peter has a better feel for this code than I do!

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

This fixes the bug.  I'm not so happy taking the other null-check removal at this stage in the game, so let's land this and I'll do a followup bug for trunk to fix the other.

Comment 12

[Boris Zbarsky [:bzbarsky]]

For what it's worth, removing the other null-check not only fixes this bug  (without the patch I attached, which I still think is still the right thing to do) but makes the assertions go away too.

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 357220 [details] [diff] [review]
Proposed fix

I think that null-check is just leftover from when DetachEditorFromWindow couldn't deal with a null nsISHEntry, but that got changed in bug 430624. I don't see a reason to keep it.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/8046c64edb06 and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cdfe7d00bf10 with reftest and such.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 357220 [details] [diff] [review]
Proposed fix

Probably worth fixing on 1.9.0 branch too.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Filed bug 474349 on comment 12.

Comment 17

[Phil Ringnalda (:philor)]

Apparently mailnews compose depended on not having the editor detached, so after this landed, it's impossible to write mail - bug 474389.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This test has failed at least once on Tinderbox:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1232474198.1232479346.15917.gz

Comment 19

[Boris Zbarsky [:bzbarsky]]

Hmm.  That failure has the test showing nothing at all in the iframe.  I'm not quite sure why that would happen, since that would mean onload fired before the data: URI loaded.  I suppose we could switch to a test where instead of listening for onload we just use the script in the iframe to signal test end so that it would time out if the script doesn't run...

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to comment #18)
> This test has failed at least once on Tinderbox
(In reply to comment #19)
> Hmm.  That failure has the test showing nothing at all in the iframe.

I can reproduce this failure (getting a blank iframe) very reliably when running reftests locally.  I see the iframe's contents when I load it normally, but it's virtually always blank (and hence fails) during reftests.

I think the problem might be that we take the reftest snapshot before the iframe has finished rendering, somehow.  If I delay the snapshot slightly by using a 0-second setTimeout() call, it reliably fixes the problem on my machine.

I'll attach a patch to that effect in a second...

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: patch to fix reftest failure

As I said above, this patch fixes the reftest failure on my machine.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Hmm.  That onload is presumably firing before we've stopped paint suppression for the subframe, right?

In which case, sounds a lot like bug 449653, except with the subframe.  But we don't reset the background-only thing for subframes, right?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 357860 [details] [diff] [review]
patch to fix reftest failure

This looks ok to get this perma-green, but still feels like a core painting bug to me...  Would love to hear roc's thoughts.

Comment 24

[Daniel Holbert [:dholbert]]

Reftest fix (attachment 357860 [details] [diff] [review]) checked in on m-c and 191:
http://hg.mozilla.org/mozilla-central/rev/5859409285d5
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f9f526b04f36

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, we only set mIsBackgroundOnly at the top level, we don't check the paint suppression status of subframes. So off the top of my head I don't know why it fails.

I wonder if this could be the same underlying problem that's making 458637-1.html randomly fail?

Daniel, if you can reproduce the failure, it would be great if you can debug it. Setting gDumpPaintList=1 would be a good start.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Let's do that in bug 474472?

I've backed the code part of this patch out of branch <http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5aa4df59fa4b> and m-c <http://hg.mozilla.org/mozilla-central/rev/4ff2e2cf9459> for now to fix bug 474389 for tomorrow's nightlies, since reviews didn't come through.

Comment 27

[Daniel Holbert [:dholbert]]

(In reply to comment #25)
> Daniel, if you can reproduce the failure, it would be great if you can debug
> it. Setting gDumpPaintList=1 would be a good start.

Sure -- I'll give that a shot tomorrow morning.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Removing the (assumed accidental) mozilla-confidential flag.

Comment 29

[Daniel Holbert [:dholbert]]

(In reply to comment #26)
> Let's do that in bug 474472?
(ah, sorry, missed that comment -- I'll direct further comments on debugging the original reftest problem to that bug.)

(In reply to comment #28)
> Removing the (assumed accidental) mozilla-confidential flag.
Weird, thanks -- I must've accidentally checked that. :)

Comment 30

[Boris Zbarsky [:bzbarsky]]

Relanded as http://hg.mozilla.org/mozilla-central/rev/3beccc95057f

Comment 31

[Boris Zbarsky [:bzbarsky]]

And http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ec8428ea990d

Comment 32

[Daniel Veditz [:dveditz]]

Comment on attachment 357220 [details] [diff] [review]
Proposed fix

Approved for 1.9.0.7, a=dveditz for release-drivers.

Comment 33

[Daniel Holbert [:dholbert]]

(In reply to comment #30)
> Relanded as http://hg.mozilla.org/mozilla-central/rev/3beccc95057f

bz: I'm confused -- it looks like you re-landed the fix this morning, but enabled the reftest yesterday as http://hg.mozilla.org/mozilla-central/rev/0c80d0556b60  with checkin-comment "This test passes now".

Why was the test passing before this re-landed?  (and given that it was passing, then does it actually test this bug?)

I'm probably just missing something -- if so, sorry. :)

Comment 34

[Boris Zbarsky [:bzbarsky]]

That reftest was fixed by either this fix or the fix for bug 474349 (see comment 12).  I'd landed that on trunk while this was backed out, and flagged the reftest as passing at the time to fix the resulting orange...

Comment 35

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.9.0 branch.

Comment 36

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.7 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.7pre) Gecko/2009021304 GranParadiso/3.0.7pre. 

Verified for 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090213 Shiretoko/3.1b3pre.

Comment 37

[Aakash Desai [:aakashd]]

Verified for trunk Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090331 Minefield/3.6a1pre