Reload of page using designMode doesn't work

RESOLVED FIXED in mozilla1.3alpha

Status

SeaMonkey
Composer
--
major
RESOLVED FIXED
15 years ago
13 years ago

People

(Reporter: mkaply, Assigned: Charles Manske)

Tracking

({regression})

Trunk
mozilla1.3alpha
regression

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: midas, URL)

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

15 years ago
In todays build (2002111404), if you go to the above URL, editor works fine, but
if you reload, it stops working.

There is an error in the Javascript console.

Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIDOMNSHTMLDocument.designMode]"  nsresult:
"0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
http://www.kaply.com/work/onepageeditor.html :: Start :: line 418"  data: no]

That line is where designMode is actually set.

Comment 1

15 years ago
This was working Monday at 8am; it was broken before Wednesday at 3am.
Keywords: nsbeta1, regression
Hardware: PC → All
Whiteboard: midas
Target Milestone: --- → mozilla1.3alpha
(Assignee)

Comment 2

15 years ago
Created attachment 106244 [details] [diff] [review]
Better cleanup in nsEditingSession v1

During reload of browser window, the DOMWindow (and associated 
command controllers?) are destroyed, but not the docshell, which owns the 
nsEditingSession. So when calling MakeWindowEditable(), we always should call
TearDownEditorOnWindow() to cleanup previous editing session.
First part of patch is in MakeWindowEditable(), second is in 
TearDownEditorOnWindow()
(Assignee)

Comment 3

15 years ago
In code I worked on recently, so I'll take the bug.
Assignee: brade → cmanske
Whiteboard: midas → midas [FIX IN HAND]need r=,sr=
(Assignee)

Comment 4

15 years ago
This fix works on both http://www.kaply.com/work/onepageeditor.html
and http://www.kaply.com/work/onepageeditor2.html
Status: NEW → ASSIGNED
(Assignee)

Comment 5

15 years ago
Comment on attachment 106244 [details] [diff] [review]
Better cleanup in nsEditingSession v1

This has turned out to be a difficult problem. There are  multiple aspects: 
nsDocShell currently leads the mEditingData object so the nsEditingSession and
nsEditor it holds doesn't get freed when a window is  destroyed. But even when
this is fixed, forced reload of a window with an editor creates a new document
and docShell, but it finds the old nsEditingSession. I added a new method,
"makeNonEditable()", to nsIEditorShell to delete the nsEditingData and called
in in an "onunload"  call in the onepageeditor's <body>, but the reload
"onload" is  still called before the old editingShell is deleted.
Attachment #106244 - Attachment is obsolete: true
(Assignee)

Comment 6

15 years ago
sorry for sloppy comment! 
"nsDocShell currently leaks the mEditingData object ..."

"I added a new method,
"makeNonEditable()", to nsIEditorDocShell to delete the nsEditingData..."
(Assignee)

Comment 7

15 years ago
Created attachment 106405 [details] [diff] [review]
Changes needed to continue v1

This doesn't entirely fix the problem, but we needed "makeNonEditible" so we
can turn off an editor and this also fixes leak of nsEditingSession and
nsEditor
in docShell.

Comment 8

15 years ago
Comment on attachment 106405 [details] [diff] [review]
Changes needed to continue v1

obsoleting patch since this was just for discussion

Charley will produce a patch with the first part of the fixes in nsDocShell.cpp
and the fixes in nsDocShellEditorData.cpp
Attachment #106405 - Attachment is obsolete: true
(Assignee)

Comment 9

15 years ago
Created attachment 106419 [details] [diff] [review]
Fix for memory leak in docShell v1

These are changes that must be fixed! (separated from "experimental" parts of
previous patch.)
(Assignee)

Updated

15 years ago
Attachment #106419 - Flags: superreview?(sfraser)
Attachment #106419 - Flags: review?(brade)

Comment 10

15 years ago
Comment on attachment 106419 [details] [diff] [review]
Fix for memory leak in docShell v1

r=brade but I'd like to see a comment in the if (mEditingSession) block that
acknowledges that mEditor->PreDestroy() is called by SetEditor(nsnull) (or
perhaps change the if (mEditor) block to just call SetEditor(nsnull)?
Attachment #106419 - Flags: review?(brade) → review+

Comment 11

15 years ago
By the way, what my investigation found is that
nsDocShellEditorData::GetOrCreateEditingSession limits the number of editing
sessions per window (to 1) essentially; this seems wrong.  I asked Charley to
ask Simon about it and continue investigating.  It seems to me that mDocShell
should be used (unless there is some case of which I'm unaware where we need to
grab the content root).

Comment 12

15 years ago
+    
+    if (mEditorData)
+    {
+      delete mEditorData;
+      mEditorData = 0;

The test is not necessary; |delete| is safe to call on null. Please use nsnull
rather than 0 to match the other lines.

+    mEditingSession->TearDownEditorOnWindow(domWindow);
+    mEditingSession = nsnull;

This is not correct.

Every docShell that has been made editable, and the docShell for the root of the
content area, have attaced nsDocShellEditorData. *Only* the content-root
docshell has an mEditingSession (as the header comments say). So you can't
assume that ever nsDocShellEditorData has mEditingSession.

To get at the correct editing session to call Teardown on, use
GetOrCreateEditingSession(). However, you don't want to make a new
nsIEditingSession just for the case of teardown, so GetOrCreateEditingSession
will need a new boolean param, 'dontCreate', that indicates that the call should
not do any creation.

Updated

15 years ago
Attachment #106419 - Flags: superreview?(sfraser) → superreview-
(Assignee)

Comment 13

15 years ago
Comment on attachment 106244 [details] [diff] [review]
Better cleanup in nsEditingSession v1

After further investigation, this patch is *exactly* the right thing to do,
imho!
Attachment #106244 - Attachment is obsolete: false
Attachment #106244 - Flags: superreview?(sfraser)
Attachment #106244 - Flags: review?(brade)
(Assignee)

Comment 14

15 years ago
I think I now understand Simon's design for attaching the nsEditingSession to 
a "root" docshell only -- this will facilitate managing > 1 editor per app window, 
such as in the case of framesets or multile editor <iframe>s per page.
So my original patch makes sense -- after a reload, the previous nsEditingSession
still exists, but is now associated with a different child docShell and 
contentWindow. So my fix clears old state data, creates new controllers, and 
editor associated with the nsEditingSession. It is definitely time to fix 
nsEditingSession to handle multiple editors!
Concerning the "Fix for memory leak" patch, I have to experiment more to figure
out the best way to do this. The patch actually does seem to work correctly 
right now, but I see Simon's points in the review.
(Assignee)

Comment 15

15 years ago
Created attachment 106704 [details] [diff] [review]
Fix detecting reuse of nsEditingSession; Fix leak in nsDocShell v2
Attachment #106244 - Attachment is obsolete: true
Attachment #106419 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #106704 - Flags: superreview?(sfraser)
Attachment #106704 - Flags: review?(brade)
(Assignee)

Comment 16

15 years ago
To address Simon's comment #12:
I'm *not* assuming that every mEditorData has an mEditingSession:
+  if (mEditingSession)
+  {
+    nsCOMPtr<nsIDOMWindow> domWindow = do_GetInterface(mDocShell);
+    // This will eventually call nsDocShellEditorData::SetEditor(nsnull)
+    //   which will call mEditorPreDestroy() and delete the editor
+    mEditingSession->TearDownEditorOnWindow(domWindow);
+    mEditingSession = nsnull;
+  }

Child DocShells will return their root docShell's nsIEditingSession when 
queried and will *not* have their own mEditorData::mEditingSession var set.
Thus we can trust mEditingSession and we don't need to call 
GetOrCreateEditingSession()
and thus do not have to add the 'dontCreate' parameter as suggested.

Comment 17

15 years ago
 nsDocShellEditorData::~nsDocShellEditorData()
 {
-  if (mEditor)
+  if (mEditingSession)
+  {
+    nsCOMPtr<nsIDOMWindow> domWindow = do_GetInterface(mDocShell);
+    // This will eventually call nsDocShellEditorData::SetEditor(nsnull)
+    //   which will call mEditorPreDestroy() and delete the editor
+    mEditingSession->TearDownEditorOnWindow(domWindow);
+    mEditingSession = nsnull;
+  }
+  else if (mEditor) // Should never have this w/o nsEditingSession!

I think this is still wrong. mEditingSession is null for all docShells except
the content-root one, so this will only ever tear down the editor for the root
window.
(Assignee)

Comment 18

15 years ago
sfraser: I understand what you mean, but this is correct in the current state in
which we only support one editor per nsEditingSession. Once I implement multiple
editor support, this will be changed to always call 
nsIEditingSession::MakeNonEditable(contextWindow) [which doesn't exist yet], which
will clean up the editor for a particular contentWindow/docShell combo.
In current state, this will simply destroy the mEditor on the context window,
which is the correct thing to do (the nsEditingSession owned by the root docShell
shouldn't be touched then.)

(Assignee)

Comment 19

15 years ago
Created attachment 106836 [details] [diff] [review]
Fix detecting reuse of nsEditingSession; Fix leak in nsDocShell v3

I added the extra param to GetOrCreateEditingSession() as sfraser requested,
but
in debugging, it doesn't seem to make any difference. I.e., during destruction
of mEditorData, the nsIEditingSession on the root docShell isn't found.
Attachment #106704 - Attachment is obsolete: true

Comment 20

15 years ago
Created attachment 106861 [details] [diff] [review]
tiny cleanup of Charley's previous patch

reverse in and out params for GetOrCreateEditingSession
Attachment #106836 - Attachment is obsolete: true
(Assignee)

Comment 21

15 years ago
I agree with brade's change in param order.

Comment 22

15 years ago
Comment on attachment 106861 [details] [diff] [review]
tiny cleanup of Charley's previous patch

+    // If we were the root docShell, destroy the editing session
+    // (it's the same as what we found above)
+    mEditingSession = nsnull;

You can just remove this;  mEditingSession is an nsCOMPtr that will get cleaned
up anyway.
Attachment #106861 - Flags: superreview+
(Assignee)

Comment 23

15 years ago
checked into 1.3a trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Whiteboard: midas [FIX IN HAND]need r=,sr= → midas

Updated

15 years ago
Attachment #106244 - Flags: superreview?(sfraser) → superreview+

Comment 24

15 years ago
Comment on attachment 106704 [details] [diff] [review]
Fix detecting reuse of nsEditingSession; Fix leak in nsDocShell v2

obsoleted by brade's patch
Attachment #106704 - Flags: superreview?(sfraser) → superreview-

Updated

15 years ago
Attachment #106244 - Flags: review?(brade) → review-

Updated

15 years ago
Attachment #106704 - Flags: review?(brade) → review-
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.