14640 - Composer can't cope with <iframe> tags

Status: VERIFIED DUPLICATE of bug 14774

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

Description

[marina]

Those three International sites are crashing when attempting to open them in
Composer by going (File|Edit page...)
http://cnn.vg.no/
http://cnn.passagen.se/
http://www.cnndanmark.dk/
Steps to reproduce:
-open http://www.cnn.com;
-on the left pane you'll see5 International sites(Spanish,Portuguese, Swedish,
Norwedgien and Danish);
-go to the Swedish site;
-go File|Edit page:
//note: while redrawing it'll crash
(Spanish and Portug are OK)

Comment 1

[buster]

M12

Comment 2

[buster]

thought about this a little more, and for the sake of stability it should be
M11.

The problem is we get 2 OnEndDocumentLoad notifications for this page.
We're prepared to handle that in nsEditorShell, except sometimes when we
get to this point we've already registered a private object as a selection
listener.  Depending on the timing, the selection could still exist, and if it
does then it'll notify all it's listeners when it changes.  But this listener
has an invalid reference to an already-destroyed editor instance.  The code
below fixes the problem by unregistering the selection listener if it was ever
registered.

Here's the fix:

Index: nsEditorShell.cpp
===================================================================
RCS file: /cvsroot/mozilla/editor/base/nsEditorShell.cpp,v
retrieving revision 1.61
diff -r1.61 nsEditorShell.cpp
744a745,759
>
>     // first, unregister the selection listener, if there was one
>     if (mStateMaintainer)
>     {
>       nsCOMPtr<nsIDOMSelection> domSelection;
>       // using a scoped result, because we don't really care if this fails
>       nsresult result = GetEditorSelection(getter_AddRefs(domSelection));
>       if (NS_SUCCEEDED(result) && domSelection)
>       {
>         domSelection->RemoveSelectionListener(mStateMaintainer);
>         NS_IF_RELEASE(mStateMaintainer);
>         mStateMaintainer = nsnull;
>       }
>     }
>

simon, please code review.  do we need the same unregistering logic in the
editor shell's destructor?

chris, I think this fix should go into M11 if simon gives the green light.

Comment 3

[buster]

simon will code review, but the root problem may actually be elsewhere.  his
call whether this code is desirable to protect ourselves from fragility in other
modules, even if that's the case.

Comment 4

[Simon Fraser [no longer active]]

http://cnn.vg.no/ doesn't crash for me. http://www.cnndanmark.dk/ does (setting
URL)

Comment 5

[Simon Fraser [no longer active]]

Well, even with buster's change, this page continually reloads in the editor. I'm
getting endless OnEndDocumentLoad notifcations.

The page contains embedded IFRAMEs, animated GIFs, and all kinds of crap, so it's
hard to say what's going on here. I filed bug 14774 on this.

I do think we need to check in buster's diff, though. That at least fixes the
crash.

Comment 6

[Simon Fraser [no longer active]]

Fix for crash checked in. there are still many problems trying to edit this page,
not least of which is that the page never stops reloading.

Comment 7

[rubydoo123]

what's the status of this bug -- is it still a crasher? THe fix was checked in
over a month ago -- please update.

Comment 8

[Simon Fraser [no longer active]]

This is still a problem -- the page continuously reloads in the editor, and
eventually crashes.

We need a reduced test case here.

Comment 9

[sujay]

Marina, can you provide a reduced test case for us?

Comment 10

[marina]

adding teruko to the cc list. Teruko, do you have some test cases in Ender group
to test what's going on when this crasher page is loading in Composer?

Comment 11

[Teruko Kobayashi]

I do not have test cases for this.  I am investigating this.

Comment 12

[rubydoo123]

using build 1999111108, I can't reproduce the crash. I talked with buster and he
thought that ftang had fixed this already. I am reassigning to Frank -- Frank,
if you fixed this already, can you please mark as fixed -- thanks.

Comment 13

[Frank Tang]

marina, please verify the status. Reopen if you still can reproduce it.

Comment 14

[Teruko Kobayashi]

Frank, I tested this in 111708 M12 build.  http://cnn.vg.no works fine.
However, when try to open as Composer http://cnn.passagen.se/ and
http://www.cnndanmark.dk/ Apprunner does not crash, but Apprunner tries to load
the page infinitely.

Comment 15

[bobj]

Does anyone have any idea why we are loading infinitely?
Has anyone been able to create a simple test case?
Need to determine how critical this is.

Comment 16

[rubydoo123]

moving to m13 and update summary to reflect the issue

Comment 17

[Gervase Markham [:gerv]]

Test case below. Composer has a problem with iframes (whatever they are ;-). See
- you don't need to know much for the BugAThon. Anyway, if you have the snippet

<iframe src="http://any.url.you.like">
</iframe>

in a web page and Edit Page, it will infinitely reload the contents of the
iframe. The test case uses http://www.mozilla.org.

So now Mozilla comes with inbuilt DOS attacks ;-)

Hope this helps to fix it.

Gerv

Comment 18

[Gervase Markham [:gerv]]

Attachment: Test case

Comment 19

[bobj]

I copied the test page to
    http://blues/users/bobj/publish/test/iframe-14640.html

Running the 1/12/2000 build (ID 2000011210), it loads fine.  No infinite loop.

Comment 20

[Gervase Markham [:gerv]]

I've just downloaded build 2000011210 (Win32) and, when I load up my local copy
of the attachment (I obviously can't get at your internal copy) it still
continuously refreshes when you go File | Edit Page and look at it in Composer
(although in M12 the title bar also changed; this is now fixed).

Are you absolutely sure you are not seeing this behaviour (as described in
[e-mail]'s comment of 9/23) in Composer?

Do you see the problem on the original page?

Also, is this now a dupe of Bug 14774?

Gerv

Comment 21

[bobj]

Whoops.  I did not read this carefully.  I tried it in brower not composer.
Yes, in browser it loops infinitely, with the console window repeating:

   The document was loaded in the content area
   WEBSHELL+ = 17
   WEBSHELL- = 18
   ...

Comment 22

[Frank Tang]

move to M14 per bobj

Comment 23

[ekrock's old account (dead)]

Bulk moving [testcase] code to new testcase keyword. Sorry for the spam!

Comment 24

[Frank Tang]

Change Plantform and OS to ALL

Comment 25

[Frank Tang]

mark this dup as 14774

*** This bug has been marked as a duplicate of 14774 ***

Comment 26

[sujay]

verified in 1/24