Last Comment Bug 351236 - Crash [@ nsGetInterface::operator()] with designMode iframes, removing styles, removing iframes, reloading, etc
: Crash [@ nsGetInterface::operator()] with designMode iframes, removing styles...
Status: RESOLVED FIXED
[sg:critical]
: crash, regression, testcase, verified1.8.0.13, verified1.8.1.5
Product: Core
Classification: Components
Component: Editor (show other bugs)
: 1.8 Branch
: All All
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug]
:
: Makoto Kato [:m_kato]
Mentors:
Depends on: 304994
Blocks: 303267
  Show dependency treegraph
 
Reported: 2006-09-03 12:03 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2009-04-24 10:59 PDT (History)
16 users (show)
dveditz: blocking1.9?
dveditz: blocking1.8.1.5+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.13+
dveditz: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.13 KB, text/html)
2006-09-03 12:04 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Patch rev. 1 (2.82 KB, patch)
2007-03-30 16:19 PDT, Mats Palmgren (:mats)
neil: review-
Details | Diff | Splinter Review
for trunk (though, there isn't a crash to fix there) (5.13 KB, patch)
2007-05-27 12:14 PDT, Olli Pettay [:smaug]
neil: review+
neil: superreview+
Details | Diff | Splinter Review
for 1.8 (5.12 KB, patch)
2007-05-27 12:15 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
a bit better one, s/GetDocShell/GetCommandUpdater/ (5.26 KB, patch)
2007-05-27 13:43 PDT, Olli Pettay [:smaug]
neil: review+
neil: superreview+
Details | Diff | Splinter Review
for 1.8 (5.24 KB, patch)
2007-05-27 14:01 PDT, Olli Pettay [:smaug]
dveditz: approval1.8.1.5+
dveditz: approval1.8.0.13+
Details | Diff | Splinter Review
Patch rev. 1, branch version (4.67 KB, patch)
2007-05-28 11:42 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2006-09-03 12:03:31 PDT
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 [:mwargers] (not working for Mozilla) 2006-09-03 12:04:42 PDT
Created attachment 236631 [details]
testcase
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-01-31 10:27:10 PST
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 2007-01-31 10:41:22 PST
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 [:mwargers] (not working for Mozilla) 2007-01-31 10:51:57 PST
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 2007-01-31 11:34:40 PST
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 [:mwargers] (not working for Mozilla) 2007-01-31 11:46:52 PST
Marking fixed by bug 243159.
Comment 7 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-03-23 04:48:05 PDT
The patch for bug 243159 that (probably) fixed this, is probably too risky for branch.
Comment 8 Daniel Veditz [:dveditz] 2007-03-29 10:33:27 PDT
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] 2007-03-30 10:27:43 PDT
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 (:mats) 2007-03-30 16:18:20 PDT
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 (:mats) 2007-03-30 16:19:29 PDT
Created attachment 260193 [details] [diff] [review]
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 [:bz] (still a bit busy) 2007-03-30 19:21:15 PDT
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 neil@parkwaycc.co.uk 2007-04-02 13:52:33 PDT
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] 2007-04-23 12:26:31 PDT
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 2007-05-24 13:49:47 PDT
mats, any ideas on how to rework the patch based on neil's comment 13
Comment 16 Olli Pettay [:smaug] 2007-05-27 12:14:48 PDT
Created attachment 266273 [details] [diff] [review]
for trunk (though, there isn't a crash to fix there)

Couldn't we just use weakptr
Comment 17 Olli Pettay [:smaug] 2007-05-27 12:15:20 PDT
Created attachment 266274 [details] [diff] [review]
for 1.8
Comment 18 neil@parkwaycc.co.uk 2007-05-27 13:41:40 PDT
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] 2007-05-27 13:43:21 PDT
Created attachment 266280 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2007-05-27 13:52:38 PDT
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] 2007-05-27 14:01:52 PDT
Created attachment 266283 [details] [diff] [review]
for 1.8
Comment 22 Mats Palmgren (:mats) 2007-05-28 06:16:45 PDT
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] 2007-05-28 06:33:07 PDT
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 neil@parkwaycc.co.uk 2007-05-28 07:14:00 PDT
(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 (:mats) 2007-05-28 08:04:06 PDT
(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 neil@parkwaycc.co.uk 2007-05-28 08:59:34 PDT
Sure: I applied the patch to branch, rebuilt in editor, launched SeaMonkey -editor but that opened an unusable window.
Comment 27 Olli Pettay [:smaug] 2007-05-28 10:21:50 PDT
Just to make it clear, the weakptr patch (attachment 266280 [details] [diff] [review]) was checked in to trunk.
Comment 28 Mats Palmgren (:mats) 2007-05-28 11:42:24 PDT
Created attachment 266408 [details] [diff] [review]
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 (:mats) 2007-05-28 11:42:58 PDT
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 neil@parkwaycc.co.uk 2007-05-28 12:32:53 PDT
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 (:mats) 2007-05-28 15:17:50 PDT
(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 neil@parkwaycc.co.uk 2007-05-28 15:24:02 PDT
(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 neil@parkwaycc.co.uk 2007-05-28 15:28:26 PDT
So, in attachment 266424 [details] [diff] [review], (why) do you still need the other PrepareForEditing?
Comment 34 Mats Palmgren (:mats) 2007-05-28 15:53:21 PDT
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 (:mats) 2007-05-28 15:58:56 PDT
... in fact, it breaks <editor> completely AFAICT.  Midas seems to work
without it though.
Comment 36 Mats Palmgren (:mats) 2007-05-28 16:24:05 PDT
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] 2007-06-15 11:21:04 PDT
Removing blocking nomination until the trunk fix/regressions are sorted out.
Comment 38 Olli Pettay [:smaug] 2007-06-15 11:29:48 PDT
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] 2007-06-22 11:18:53 PDT
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] 2007-06-28 11:45:56 PDT
Still need an answer about whether the approval request is really on the intended patch.
Comment 41 Olli Pettay [:smaug] 2007-07-02 06:17:58 PDT
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] 2007-07-02 10:40:37 PDT
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] 2007-07-02 11:39:21 PDT
(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] 2007-07-02 13:24:27 PDT
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] 2007-07-04 13:19:49 PDT
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] 2007-08-20 10:43:22 PDT
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:] 2009-04-24 10:59:03 PDT
crash test landed
http://hg.mozilla.org/mozilla-central/rev/2bd30f8620e3

Note You need to log in before you can comment on or make changes to this bug.