Closed Bug 336104 Opened 16 years ago Closed 12 years ago

"ASSERTION: no frame, see bug #188946"

Categories

(Core :: DOM: Editor, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5
Tracking Status
status1.9.2 --- .9-fixed
status1.9.1 --- wontfix

People

(Reporter: jruderman, Assigned: ehsan.akhgari)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 5 obsolete files)

###!!! ASSERTION: no frame, see bug #188946: 'frame', file /Users/admin/trunk/mozilla/editor/libeditor/base/nsEditor.cpp, line 4185

The comments near this assertion suggest that it is a bogus assertion.
Attached file testcase (obsolete) —
Blocks: 336383
Now that bug 47903 is fixed, the old testcase just hits WRONG_DOCUMENT_ERR.  Here's an updated testcase that still demonstrates the bug.
Attachment #220391 - Attachment is obsolete: true
QA Contact: editor
Assignee: mozeditor → nobody
I can think of a few possibilities here:
 * remove the assertion
 * use nsComputedDOMStyle::GetStyleContextForElement or nsComputedDOMStyle::GetStyleContextForElementNoFlush
 * ensure we don't call this on nodes without frames (perhaps by having something higher on the stack flush?)

This is the top assertion in reftests/crashtests.
Investigating.  I think the second solution is the way to go here (and we also need to remove the assertion for sure!).
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attached patch Patch (v1) (obsolete) — Splinter Review
Should I just include Jesse's test case in the unit test suite hoping for us to actually count assertions as failures in mochitests some day?
Attachment #443509 - Flags: review?(roc)
So this is going to flush right? Are the editor callers of this method prepared to handle frame destruction?
(In reply to comment #6)
> So this is going to flush right? Are the editor callers of this method prepared
> to handle frame destruction?

You're right, and no, they aren't.  Is it safe to use the NoFlush variant instead?
(This is especially risky because frame destruction could lead to the editor itself being destructed as well.)
Maybe something higher up the stack should flush and check for the existence of a frame?
It's not immediately obvious to me at what level that check is meaningful.  It probably doesn't make sense to do this from the immediate callers, and as we go higher on the stack, I'm worried that we might miss a few of the callers.

That said, most of the callers into the editor code that I've seen right now assume that editor can flush at arbitrary point, and protect against it using weak frame checks, but no code inside the editor currently does those checks.
Attached patch Patch (v2) (obsolete) — Splinter Review
Use the "noflush" variant.  This passes a full try unit test run.
Attachment #443509 - Attachment is obsolete: true
Attachment #443747 - Flags: review?(roc)
Attachment #443509 - Flags: review?(roc)
dbaron, does GetStyleContextForElementNoFlush actually depend on style changes and frames having been previously flushed? Or is it only a content flush that we're avoiding here? Doesn't the HTML5 parser actually make content flushes a non-issue?
dbaron: please see comment 12.
Blocks: 564461
Attachment #443747 - Flags: feedback?(dbaron)
Comment on attachment 443747 [details] [diff] [review]
Patch (v2)

So... there is a possibility that a content flush would change the result by changing, e.g., what selectors match.

There's also the possibility that there are queued style changes (since we process style changes asynchronously).  In that case, you'd get the old data -- unless you're inside something that doesn't get a frame, in which case you'd get a mix of old and new data if there was actually no flush (since you'd find the nearest ancestor with a frame and then create new style data for the path from there to the target, but inheriting from the old data).

That said, none of this sounds particularly problematic to me, so I think it's ok.

But I've cc:ed bz so he can have a chance to object too.
Attachment #443747 - Flags: feedback?(dbaron) → feedback+
> So... there is a possibility that a content flush would change the result by
> changing, e.g., what selectors match.

Content flushes don't (normally) change the DOM.  They just dispatch delayed append/insert notifications.

Getting old or partially-old data doesn't seem like a problem as long as we have a mechanism to "fix" things when style data changes, right?
(In reply to comment #16)
> Getting old or partially-old data doesn't seem like a problem as long as we
> have a mechanism to "fix" things when style data changes, right?

Well, "fixing" things here might not be as easy as it sounds.  :-)  Do we have any way to force the style changes to be processed on a node without doing a content flush?
No, but why would such a thing help you?
It should make sure that we have a style context based on the style changes happened so far, doesn't it?
Yes, but why is the "without doing a content flush" part useful?
See comment 6 and 7.
But style change processing on its own can run arbitrary script; hence my questions...
Ah, ok, in that case, I rest my case.

Given the information at hand, I think it's safe for us to take this patch.  Let's see what roc thinks.
The question is whether all the stuff that uses "IsPreformatted" is going to be rerun/fixed up when the style changes. I think the answer is "no", because IsPreformatted is used to make decisions about how to modify the DOM in response to editor commands. So I think we do need to flush here, otherwise applying the same editor command to the same DOM will produce different results depending on whether layout has been flushed or not.

I think here we need to have editor flush frames along all paths leading to IsPreformatted calls --- or flush in IsPreformatted itself.
(In reply to comment #24)
> I think here we need to have editor flush frames along all paths leading to
> IsPreformatted calls --- or flush in IsPreformatted itself.

We have to do this where we can actually handle frame destruction in a sane way.  IsPreformatted is called for a lot of editor operations (for example, for inserting text), and there's no good way of handling frame destruction inside the function itself.  There isn't that much we can do in the callers of IsPreformatted as well, so we basically need to climb the call chain to reach somewhere better, which is probably editor callers.

Do you know of a better way to handle this, or is this the only option?
Editor mostly works with DOM nodes, not frames. I think we should be able to flush in most places inside editor.
(In reply to comment #26)
> Editor mostly works with DOM nodes, not frames. I think we should be able to
> flush in most places inside editor.

Well, I'm kind of puzzled here.  I was talking about whether the callers are prepared to handle frame destruction (comment 6).
Generally, if the callers aren't holding pointers to frames then they are able to handle frame destruction.
(In reply to comment #28)
> Generally, if the callers aren't holding pointers to frames then they are able
> to handle frame destruction.

I tried to take an exhaustive look at all the call chains which might lead to IsPreformatted to be called.  The nsTextControlFrame's call sites are protected by weak frame checks.  This code can also be triggered in numerous ways by content/chrome scripts, as well as the editor event handlers (for things such as typing text and pasting), but I assume that those are safe with regard to frames.  If I'm not mistaken, I think we can take the v1 patch for this bug after all.
Attachment #443747 - Flags: review?(roc)
http://hg.mozilla.org/mozilla-central/rev/0d2f650b8581
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
OS: Mac OS X → All
Hardware: PowerPC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Attachment #443509 - Flags: approval1.9.2.5?
Attachment #443509 - Flags: approval1.9.1.11?
Attachment #443747 - Attachment is obsolete: true
Backed out because of crashtest failure:

http://hg.mozilla.org/mozilla-central/rev/6ef424688579

Here's the crash stack:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1274466949.1274467091.2801.gz

Investigating...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #443509 - Flags: approval1.9.2.5?
Attachment #443509 - Flags: approval1.9.1.11?
When you reland this, you'll need to remove at least the following annotations:
http://mxr.mozilla.org/mozilla-central/search?string=bug%20336104

You'll probably want to check that against try server, after merging on top of http://hg.mozilla.org/mozilla-central/rev/36b9855dad18
OK, figured out what caused the crash.

Basically, after nsIPlaintextEditor::InsertText is called, we can't touch the frame or its content node without the weak frame check.  Bug 518122 adds a call to IsSingleLineTextControl before that <http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsTextControlFrame.cpp#2851>, which blows away when trying to access mContent.

Patch forthcoming.
Attached patch Patch (v1.1) (obsolete) — Splinter Review
This fixes the crash, corrects the assertion annotations inside manifest files, and adds Jesse's test case to the crashtest suite as an assertion test.  I've pushed this to the try server to make sure that all is green with this version of the patch.
Attachment #443509 - Attachment is obsolete: true
Attachment #446811 - Flags: review?(roc)
Attached patch Patch (v1.2)Splinter Review
Removed two more assertions which were fixed from the manifests.
Attachment #446811 - Attachment is obsolete: true
Attachment #446863 - Flags: review?(roc)
Attachment #446811 - Flags: review?(roc)
Comment on attachment 446863 [details] [diff] [review]
Patch (v1.2)

r=dbaron on the differences from the last patch
Attachment #446863 - Flags: review?(roc) → review+
It seems to me that bailing out in SetValue here would lead to failure to set the value if there's something that causes the frame for the text input to be reconstructed. Right?

But I think/hope that moving the editor into the element will fix this?
(In reply to comment #38)
> It seems to me that bailing out in SetValue here would lead to failure to set
> the value if there's something that causes the frame for the text input to be
> reconstructed. Right?

If you mean the value setter throwing in js, then no, because the return value from nsTextControlFrame::SetValue is not propagated.  The value itself would be preserved as well, because when the frame is being destroyed, the value is transferred to the content, and when the new frame is constructed, the value is transferred back to the frame, and InsertText would end up modifying the new anonymous div, which means that when SetValue returns, the correct value is stored inside the anonymous content.

> But I think/hope that moving the editor into the element will fix this?

Moving the editor to the element will preserve this behavior.  But now that you've mentioned it, this is probably something which we want to test to make sure that it doesn't break in the future.  I've added a reftest to my patch in bug 534785 for this.
http://hg.mozilla.org/mozilla-central/rev/37b7ca43fafe
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 568362
Attachment #446863 - Flags: approval1.9.2.7?
Attachment #446863 - Flags: approval1.9.1.12?
Comment on attachment 446863 [details] [diff] [review]
Patch (v1.2)

Approved for 1.9.2.9 and 1.9.1.12, a=dveditz for release-drivers
Attachment #446863 - Flags: approval1.9.2.8?
Attachment #446863 - Flags: approval1.9.2.8+
Attachment #446863 - Flags: approval1.9.1.12?
Attachment #446863 - Flags: approval1.9.1.12+
I just realized that nsComputedDOMStyle::GetStyleContextForElement is not available on branches.  Dbaron, is there a replacement API which I can use?
The branch function to be used is nsInspectorCSSUtils::GetStyleContextForContent.
Attached patch Regression fix for 1.9.2 (obsolete) — Splinter Review
The 1.9.2 branch went orange on http://mxr.mozilla.org/mozilla1.9.2/source/layout/generic/test/test_bug527306.html.  I basically had landed a fix for that on trunk, and I had to write a less clean but effectively equivalent patch for 1.9.2.  It basically sets the value on the content node if the frame gets destroyed during nsIEditor::InsertText call.
Attachment #460585 - Flags: review?(jst)
Attachment #460585 - Flags: approval1.9.2.9?
Comment on attachment 460585 [details] [diff] [review]
Regression fix for 1.9.2

+        nsCOMPtr<nsIDOMHTMLInputElement> inputElement = do_QueryInterface(GetContent());
+        nsCOMPtr<nsIDOMHTMLTextAreaElement> textAreaElement = do_QueryInterface(GetContent());
+        NS_ASSERTION(inputElement || textAreaElement,
+                     "The content node must either be an input element or a textarea");
+
         if (currentValue.Length() < 1)
           editor->DeleteSelection(nsIEditor::eNone);
         else {
           if (plaintextEditor)
             plaintextEditor->InsertText(currentValue);
         }
-        NS_ENSURE_STATE(weakFrame.IsAlive());
+        if (!weakFrame.IsAlive()) {
+          if (inputElement) {
+            return inputElement->SetValue(currentValue);
+          } else if (textAreaElement) {
+            return textAreaElement->SetValue(currentValue);
+          }

Given the cost of QI, if it matters here you could simply grab a strong reference to the content above, and only do QI in the case where the frame is no longer alive, and you could QI to nsIDOMHTMLInputElement first, and only if that fails QI to nsIDOMHTMLTextAreaElement.

r=jst either way, but please do remove the else-after-return.
Attachment #460585 - Flags: review?(jst) → review+
With comments addressed.
Attachment #460585 - Attachment is obsolete: true
Attachment #460602 - Flags: approval1.9.2.9?
Attachment #460585 - Flags: approval1.9.2.9?
Comment on attachment 460602 [details] [diff] [review]
Regression fix for 1.9.2

a=beltzner
Attachment #460602 - Flags: approval1.9.2.9? → approval1.9.2.9+
Bustage fix for 1.9.2 landed as:

<http://hg.mozilla.org/releases/mozilla-1.9.2/rev/177e3561f58c>
I was looking into fixing the bustages on 1.9.1 after landing of this patch.  Basically, we'll need to backport too much stuff to make the landing of this patch safe, and I think that the risk of backporting all those fixes to that branch outweighs the benefits of taking this single bug on that branch, therefore I've backed out the patch from 1.9.1, and I'm setting status1.9.1 to wontfix.

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b0f1a3ebb20f

If you have any objections, please feel free to comment on the bug.
I believe this bug may have broken signatures on the Thunderbird 3.1 branch: 

http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird3.1/1280261643.1280265110.27192.gz

(windows & mac)

Sorry for direct comment I've only just seen it and I need to be off for a while now.
(In reply to comment #51)
> I believe this bug may have broken signatures on the Thunderbird 3.1 branch: 
> 
> http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird3.1/1280261643.1280265110.27192.gz
> 
> (windows & mac)
> 
> Sorry for direct comment I've only just seen it and I need to be off for a
> while now.

Can you please file a new bug with more details on the signature problem?  Does that work correctly on mozilla-central?
(In reply to comment #52)
> (In reply to comment #51)
> > I believe this bug may have broken signatures on the Thunderbird 3.1 branch: 
> > 
> > http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird3.1/1280261643.1280265110.27192.gz
> 
> Can you please file a new bug with more details on the signature problem?  Does
> that work correctly on mozilla-central?

It appears that the unit tests were different between trunk and 1.9.2, and I fixed the tests with applying the differences to 1.9.2:

http://hg.mozilla.org/releases/comm-1.9.2/rev/b9b4943efac8
(In reply to comment #53)
> It appears that the unit tests were different between trunk and 1.9.2, and I
> fixed the tests with applying the differences to 1.9.2:
> 
> http://hg.mozilla.org/releases/comm-1.9.2/rev/b9b4943efac8

Phew!  I was afraid that I've caused yet another regression for Thunderbird!  :-)
Depends on: 586389
Attachment #446863 - Flags: approval1.9.1.12+ → approval1.9.1.12-
Depends on: 592592
Depends on: 595197
Depends on: 596001
You need to log in before you can comment on or make changes to this bug.