The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
Editor
--
critical
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: smaug)

Tracking

(5 keywords)

1.8 Branch
crash, regression, testcase, verified1.8.0.13, verified1.8.1.5
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 ?
blocking1.8.1.5 +
wanted1.8.1.x +
blocking1.8.0.13 +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical], crash signature)

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Created attachment 236631 [details]
testcase
(Reporter)

Comment 2

10 years ago
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

10 years ago
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.
(Reporter)

Comment 4

10 years ago
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

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

Comment 6

10 years ago
Marking fixed by bug 243159.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Depends on: 243159
Resolution: --- → FIXED
Flags: blocking1.8.1.4?
(Reporter)

Comment 7

10 years ago
The patch for bug 243159 that (probably) fixed this, is probably too risky for branch.
This is a presumably exploitable crash using freed objects, is there a smaller fix that might work for this specific testcase?
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.4?
Whiteboard: [sg:critical] freed memory
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Assignee: nobody → mats.palmgren
Status: REOPENED → NEW
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
Whiteboard: [sg:critical] freed memory → [sg:critical] freed memory. fixed on trunk by 243159
Version: Trunk → 1.8 Branch
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.
Blocks: 303267
No longer depends on: 243159
OS: Windows XP → All
Hardware: PC → All
Whiteboard: [sg:critical] freed memory. fixed on trunk by 243159 → [sg:critical] freed memory
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".
Attachment #260193 - Flags: superreview?(bzbarsky)
Attachment #260193 - Flags: review?(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...

Updated

10 years ago
Attachment #260193 - Flags: superreview?(bzbarsky)
Attachment #260193 - Flags: review?(neil)
Attachment #260193 - Flags: review?(bzbarsky)

Comment 13

10 years ago
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.
Attachment #260193 - Flags: review?(neil) → review-
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
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.12+

Comment 15

10 years ago
mats, any ideas on how to rework the patch based on neil's comment 13
(Assignee)

Comment 16

10 years ago
Created attachment 266273 [details] [diff] [review]
for trunk (though, there isn't a crash to fix there)

Couldn't we just use weakptr
Attachment #266273 - Flags: superreview?(neil)
Attachment #266273 - Flags: review?(neil)
(Assignee)

Comment 17

10 years ago
Created attachment 266274 [details] [diff] [review]
for 1.8

Comment 18

10 years ago
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?
Attachment #266273 - Flags: superreview?(neil)
Attachment #266273 - Flags: superreview+
Attachment #266273 - Flags: review?(neil)
Attachment #266273 - Flags: review+
(Assignee)

Comment 19

10 years ago
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.
Attachment #266273 - Attachment is obsolete: true
Attachment #266274 - Attachment is obsolete: true
Attachment #266280 - Flags: superreview?(neil)
Attachment #266280 - Flags: review?(neil)

Comment 20

10 years ago
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].
Attachment #266280 - Flags: superreview?(neil)
Attachment #266280 - Flags: superreview+
Attachment #266280 - Flags: review?(neil)
Attachment #266280 - Flags: review+
(Assignee)

Updated

10 years ago
Assignee: mats.palmgren → Olli.Pettay
(Assignee)

Comment 21

10 years ago
Created attachment 266283 [details] [diff] [review]
for 1.8
Attachment #266283 - Flags: approval1.8.1.5?
Attachment #266283 - Flags: approval1.8.0.13?
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 23

10 years ago
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

10 years ago
(In reply to comment #22)
>Please explain what you think is wrong with it
It breaks <editor>. I can't edit anything.
(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

10 years ago
Sure: I applied the patch to branch, rebuilt in editor, launched SeaMonkey -editor but that opened an unusable window.
(Assignee)

Comment 27

10 years ago
Just to make it clear, the weakptr patch (attachment 266280 [details] [diff] [review]) was checked in to trunk.
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]).
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

10 years ago
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
(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.
Depends on: 304994

Comment 32

10 years ago
(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

10 years ago
So, in attachment 266424 [details] [diff] [review], (why) do you still need the other PrepareForEditing?
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.
... in fact, it breaks <editor> completely AFAICT.  Midas seems to work
without it though.
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.
Flags: blocking1.9?
Removing blocking nomination until the trunk fix/regressions are sorted out.
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
(Assignee)

Comment 38

10 years ago
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.
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?
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13+
Still need an answer about whether the approval request is really on the intended patch.
Whiteboard: [sg:critical] freed memory → [sg:critical] need answer to comment 39
(Assignee)

Comment 41

10 years ago
It is on the intended patch, unless someone comes with a regression-free branch patch.
(Bug 304994 hasn't landed even trunk yet.)
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?
Attachment #266283 - Flags: approval1.8.1.5?
Attachment #266283 - Flags: approval1.8.1.5+
Attachment #266283 - Flags: approval1.8.0.13?
Attachment #266283 - Flags: approval1.8.0.13+
(Assignee)

Comment 43

10 years ago
(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).
(Assignee)

Comment 44

10 years ago
Checked in the weakptr patch to branches. That fixes the crash.
Marking this fixed, but should a new one opened for the other fixes.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Keywords: fixed1.8.0.13, fixed1.8.1.5
Resolution: --- → FIXED
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
Keywords: fixed1.8.1.5 → verified1.8.1.5
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.
Keywords: fixed1.8.0.13 → verified1.8.0.13
Group: security
Whiteboard: [sg:critical] need answer to comment 39 → [sg:critical]
Flags: in-testsuite?

Comment 47

8 years ago
crash test landed
http://hg.mozilla.org/mozilla-central/rev/2bd30f8620e3
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsGetInterface::operator()]
You need to log in before you can comment on or make changes to this bug.