351236 - Crash [@ nsGetInterface::operator()] with designMode iframes, removing styles, removing iframes, reloading, etc

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase, which crashes for me within 2 seconds after loading.

Talkback ID: TB22850410W
0x02769249
nsGetInterface::operator()  [mozilla\xpcom\build\nsiinterfacerequestorutils.cpp, line 52]
nsCOMPtr_base::assign_from_helper  [mozilla\xpcom\build\nscomptr.cpp, line 150]
nsCOMPtr<nsICommandManager>::nsCOMPtr<nsICommandManager>  [mozilla\dist\include\xpcom\nscomptr.h, line 696]
nsComposerCommandsUpdater::TimerCallback  [mozilla\editor\composer\src\nscomposercommandsupdater.cpp, line 279]
nsComposerCommandsUpdater::Notify  [mozilla\editor\composer\src\nscomposercommandsupdater.cpp, line 399]
NS_ProcessNextEvent_P  [mozilla\xpcom\build\nsthreadutils.cpp, line 225]
nsBaseAppShell::Run  [mozilla\widget\src\xpwidgets\nsbaseappshell.cpp, line 153]
0x00f8296c
0xccccc3c0

This is a regression, doesn't happen with a 2005-08-12 build, happens with a 2005-08-13 build:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-08-12+06&maxdate=2005-08-13+09&cvsroot=%2Fcvsroot
Not really sure which patch could have caused this.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

This has become worksforme between 2007-01-02 and 2007-01-03:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-01-02+04&maxdate=2007-01-03+06&cvsroot=%2Fcvsroot
I can still see the crash on a current branch build.
So I guess this is fixed somehow with the patch from bug 243159?
There were some other possible checkins, but those also got checked in on branch.

Comment 3

[Bernd]

bug 243159 was checked in twice:

1.1293	bmlk%gmx.de	2007-01-02 23:18	 	remove TableProcessChild, this creates a single codepath trough ProcessChildren for frame construction bug 243159 2. attempt r=rbs sr=bz
1.1292	bmlk%gmx.de	2006-12-29 03:27	 	backout of bug 243159, rtest is necessary before checkin
1.1291	bmlk%gmx.de	2006-12-27 06:00	 	remove TableProcessChild, this creates a single codepath trough ProcessChildren for frame construction bug 243159 r=rbs sr=bz

so there is one build that has the patch already before.

Comment 4

[Martijn Wargers (dead)]

Thanks!
I don't crash with a 2006-12-28 build, but I do crash with a 2006-12-29 build, so this bug is somehow fixed by the fix from bug 243159, I would say.

Comment 5

[Bernd]

That makes sense, with the new patch all the table display types on things that can't be table parts are basically NOOPs.

Comment 6

[Martijn Wargers (dead)]

Marking fixed by bug 243159.

Comment 7

[Martijn Wargers (dead)]

The patch for bug 243159 that (probably) fixed this, is probably too risky for branch.

Comment 8

[Daniel Veditz [:dveditz]]

This is a presumably exploitable crash using freed objects, is there a smaller fix that might work for this specific testcase?

Comment 9

[Daniel Veditz [:dveditz]]

Mats: Bernd has said he's too busy, can you look into whether there's a possibility of a small band-aide patch for the branch for just the security bug that doesn't involve landing the entire fix for bug 249159. Or decide if we should just go for it and land that anyway.

Comment 10

[Mats Palmgren (inactive)]

It's very unlikely that bug 243159 fixed this (which is a frame constructor
change that deals with pseudo table frames).
The crash here involves editor and docshell code only.
I think it's mostly luck this does not occur on trunk (before bug 357861).
From the regression range in comment 0 I would guess it's a regression
from bug 303267.

Comment 11

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

In nsEditingSession::PrepareForEditing we set 'mDoneSetup' to true,
register a web progress listener (unconditionally) and set
'mProgressListenerRegistered' to true
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/editor/composer/src/nsEditingSession.cpp&rev=1.43.4.1&root=/cvsroot&mark=1267-1268,1270,1284#1265

But in nsEditingSession::TearDownEditorOnWindow we only
remove the listener if "designMode=on".
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/editor/composer/src/nsEditingSession.cpp&rev=1.43.4.1&root=/cvsroot&mark=494-495,523-533#492

The crash happens because when 'isMidas' is false we leave
mProgressListenerRegistered true which makes us do an early
return in PrepareForEditing() which leaves 'mDoneSetup'
false which means we will return early in TearDownEditorOnWindow
for the lifetime if this object. The crash is inevitable, in
this case it's a nsComposerCommandsUpdater that gets a timer
callback and it uses a docshell that is destroyed.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/editor/composer/src/nsComposerCommandsUpdater.h&rev=MOZILLA_1_8_BRANCH&root=/cvsroot&mark=119#58

The fix is to always remove the listener if we have one and set
mProgressListenerRegistered to false so that we will enter
PrepareForEditing and set mDoneSetup properly the next time,
allowing TearDownEditorOnWindow to unregister stuff.

There is also a patch in bug 357861 that untangles these two
bits, which makes sense, but I think unregistering the listener
should not depend on "designMode=on".

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 260193 [details] [diff] [review]
Patch rev. 1

I'm really not the right reviewer for this code.  Neil or Simon are better bets by far...

Comment 13

[[e-mail]]

Comment on attachment 260193 [details] [diff] [review]
Patch rev. 1

(In reply to comment #11)
>There is also a patch in bug 357861 that untangles these two
>bits, which makes sense, but I think unregistering the listener
>should not depend on "designMode=on".
There is a reason for this: we don't want to unregister progress listeners for <editor> otherwise you'll never be able to edit anything. At least, I think that's why I fixed bug 357861 in the way I did.

Comment 14

[Daniel Veditz [:dveditz]]

This doesn't have a trunk fix, we'll need to bake it there first so this isn't going to make this set of releases. Moving nomination to 1.8.1.5

Comment 15

[chris hofmann]

mats, any ideas on how to rework the patch based on neil's comment 13

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for trunk (though, there isn't a crash to fix there)

Couldn't we just use weakptr

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for 1.8

Comment 18

[[e-mail]]

Comment on attachment 266273 [details] [diff] [review]
for trunk (though, there isn't a crash to fix there)

>+already_AddRefed<nsIDocShell>
>+nsComposerCommandsUpdater::GetDocShell()
>+{
>+  nsCOMPtr<nsIDocShell> docShell = do_QueryReferent(mDocShell);
>+  nsIDocShell* shell = nsnull;
>+  docShell.swap(shell);
>+  NS_WARN_IF_FALSE(shell, "Null DocShell!");
>+  return shell;
>+}
This is pointless as it stands, it would be cheaper to inline
nsCOMPtr<nsIDocShell> docShell(do_QueryReferent(mDocShell), &rv);
NS_ENSURE_SUCCESS(rv, rv);
Of course you could always enhance it to return the nsPICommandUpdater.

>   nsIDOMWindow* mDOMWindow;  // Weak reference
>-  nsIDocShell*  mDocShell;   // Weak reference
>+  nsWeakPtr     mDocShell;
At least on trunk, can we clean this up and just keep a weak ref to the window?

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Attachment: a bit better one, s/GetDocShell/GetCommandUpdater/

The branch patch has only one small change:
branch
-    mDocShell = scriptObject->GetDocShell();
+    mDocShell = do_GetWeakReference(scriptObject->GetDocShell());
trunk
-    mDocShell = window->GetDocShell();
+    mDocShell = do_GetWeakReference(window->GetDocShell());

To make sure that this doesn't cause regressions, docshell is retrieved in ::Init, not every time from mDOMWindow.
Possible followup patch to change that in trunk.

Comment 20

[[e-mail]]

Comment on attachment 266280 [details] [diff] [review]
a bit better one, s/GetDocShell/GetCommandUpdater/

r+sr=me based on the interdiff from attachment 266273 [details] [diff] [review].

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for 1.8

Comment 22

[Mats Palmgren (inactive)]

Sorry but I think you're just wallpapering over the real bug here.
The whole purpose of TearDownEditorOnWindow() is to remove all
associations between the editor and the window.  Leaving the
progress listeners (that was added in PrepareForEditing) still
running in some cases in simply wrong.  This is what my patch
fixed.  With that fixed there is no need for adding WeakPtrs
because when TearDownEditorOnWindow() functions correctly
the mDocShell pointer will never be null.  We have a serious
programming error if that ever happens and I really don't think
we should wallpaper over it.

To answer Neil's question in comment 13: I think you have misunderstood
TearDownEditorOnWindow().  I've tested my patch quite a bit and I saw no
regressions. Please explain what you think is wrong with it (see
explanation in comment 11).

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

With the weakptr patch I wanted to minimize the risk of regressions in 
the branches. IMO, we have had way too many regressions in 1.8.1.x.
attachment 260193 [details] [diff] [review] is perhaps the right thing to do, but do we really 
need it in the branches, or is it really regression-risk-free?

Comment 24

[[e-mail]]

(In reply to comment #22)
>Please explain what you think is wrong with it
It breaks <editor>. I can't edit anything.

Comment 25

[Mats Palmgren (inactive)]

(In reply to comment #24)
> It breaks <editor>. I can't edit anything.

I'm using the editor in SeaMonkey and that works fine for me on Linux.
Could you provide details on what you're doing that breaks?

Comment 26

[[e-mail]]

Sure: I applied the patch to branch, rebuilt in editor, launched SeaMonkey -editor but that opened an unusable window.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Just to make it clear, the weakptr patch (attachment 266280 [details] [diff] [review]) was checked in to trunk.

Comment 28

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1, branch version

Ok, so the difference in behavior between trunk/branch is bug 357861 -
setting the |mDoneSetup| flag in PrepareForEditing() is premature and
it makes StartDocumentLoad() blow away what has been setup already
(deregistering progress listeners). Sorry for not seeing that earlier.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/editor/composer/src/nsEditingSession.cpp&rev=MOZILLA_1_8_BRANCH&root=/cvsroot&mark=1041#1025

This is the same as Patch rev. 1 but with bug 357861 included
(attachment 253616 [details] [diff] [review]).

Comment 29

[Mats Palmgren (inactive)]

At this point I'm not sure which patch is less risky for branch, but for
trunk I still think my first patch is the way to go - we should untangle
the nsEditingSession states and get it right and then we don't need
to use WeakPtrs.

Comment 30

[[e-mail]]

Comment on attachment 266408 [details] [diff] [review]
Patch rev. 1, branch version

Almost there...
Works: seamonkey -edit <url>
Works: seamonkey -editor
Does not work: seamonkey -editor then manually opening a page or url

Comment 31

[Mats Palmgren (inactive)]

(In reply to comment #30)
> Does not work: seamonkey -editor then manually opening a page or url

Nice catch.  Yes, the proposed TearDownEditorOnWindow() change extends
bug 304994 to affect this case too.  Applying the patch in that bug
makes it work properly, also on branch with the attached branch patch
in this bug.

Comment 32

[[e-mail]]

(In reply to comment #31)
>(In reply to comment #30)
>>Does not work: seamonkey -editor then manually opening a page or url
>Nice catch.  Yes, the proposed TearDownEditorOnWindow() change extends
>bug 304994 to affect this case too.  Applying the patch in that bug
>makes it work properly, also on branch with the attached branch patch
>in this bug.
Yes, with 260193, 253616 and 266424 applied everything still works.

Comment 33

[[e-mail]]

So, in attachment 266424 [details] [diff] [review], (why) do you still need the other PrepareForEditing?

Comment 34

[Mats Palmgren (inactive)]

Removing the PrepareForEditing() call in MakeWindowEditable() breaks:
seamonkey -edit URL
because we never setup a progress listener in that case and thus
never reach StartDocumentLoad() or any other notification callback.

Comment 35

[Mats Palmgren (inactive)]

... in fact, it breaks <editor> completely AFAICT.  Midas seems to work
without it though.

Comment 36

[Mats Palmgren (inactive)]

Patch rev. 2 in bug 304994 seems to work correctly.  I have to admit
I don't quite understand what that |aDoAfterUriLoad| arg means though.
I mostly just guessed from the comment.  Anyway, just adding it FYI.

Comment 37

[Daniel Veditz [:dveditz]]

Removing blocking nomination until the trunk fix/regressions are sorted out.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Mats, Neil, which patch should be taken to branch?
The one using nsWeakPtr (and which landed on trunk) is at least pretty
safe, but ofc if there is a better, but still regression-safe patch, that should be taken.

Comment 39

[Daniel Veditz [:dveditz]]

The approval requests appear to be on not the latest patch. Is this really the one you want to land on branch? Have the regressions been sorted out?

Comment 40

[Daniel Veditz [:dveditz]]

Still need an answer about whether the approval request is really on the intended patch.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

It is on the intended patch, unless someone comes with a regression-free branch patch.
(Bug 304994 hasn't landed even trunk yet.)

Comment 42

[Daniel Veditz [:dveditz]]

Comment on attachment 266283 [details] [diff] [review]
for 1.8

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers
Do we also need to approve the v2 patch in bug 304994?

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #42)
> (From update of attachment 266283 [details] [diff] [review])
> approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers
> Do we also need to approve the v2 patch in bug 304994?
> 

Not with my patch, but apparently if Mats' patch was used, 
also bug 304994 would be needed (if I've understood that correctly).

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Checked in the weakptr patch to branches. That fixes the crash.
Marking this fixed, but should a new one opened for the other fixes.

Comment 45

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.5 using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.5pre) Gecko/2007070403 BonEcho/2.0.0.5pre (Fedora F7) and Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.5pre) Gecko/20070704 BonEcho/2.0.0.5pre ID:2007070403 - no crash on testcase - adding verified keyword

Comment 46

[juan becerra [:juanb]]

Verified on MacIntel using Thunderbird version 1.5.0.13 (20070809). Enabled JavaScript, saved the testcase and served it from my local web server, then I set the Start Page to the testcase URL. 1.5.0.12 crashes, but 1.5.0.13 does not.

Comment 47

[Bob Clary [:bc] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/2bd30f8620e3