"ASSERTION: no frame, see bug #188946"

RESOLVED FIXED in mozilla1.9.3a5

Status

()

RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: jruderman, Assigned: Ehsan)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla1.9.3a5
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(status1.9.2 .9-fixed, status1.9.1 wontfix)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

13 years ago
###!!! 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.
(Reporter)

Comment 1

13 years ago
Created attachment 220391 [details]
testcase
(Reporter)

Updated

13 years ago
Blocks: 336383
(Reporter)

Comment 2

12 years ago
Created attachment 256267 [details]
testcase with adoptNode

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

Comment 4

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

Comment 5

9 years ago
Created attachment 443509 [details] [diff] [review]
Patch (v1)

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?
(Assignee)

Comment 7

9 years ago
(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?
(Assignee)

Comment 8

9 years ago
(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?
(Assignee)

Comment 10

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

Comment 11

9 years ago
Created attachment 443747 [details] [diff] [review]
Patch (v2)

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?
(Assignee)

Comment 13

9 years ago
dbaron: please see comment 12.
(Assignee)

Updated

9 years ago
Blocks: 564461
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?
(Assignee)

Comment 17

9 years ago
(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?
(Assignee)

Comment 19

9 years ago
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?
(Assignee)

Comment 21

9 years ago
See comment 6 and 7.
But style change processing on its own can run arbitrary script; hence my questions...
(Assignee)

Comment 23

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

Comment 25

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

Comment 27

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

Comment 29

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

Updated

9 years ago
Attachment #443747 - Flags: review?(roc)
(Assignee)

Comment 31

9 years ago
http://hg.mozilla.org/mozilla-central/rev/0d2f650b8581
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
OS: Mac OS X → All
Hardware: PowerPC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
(Assignee)

Updated

9 years ago
Attachment #443509 - Flags: approval1.9.2.5?
Attachment #443509 - Flags: approval1.9.1.11?
(Assignee)

Updated

9 years ago
Attachment #443747 - Attachment is obsolete: true
(Assignee)

Comment 32

9 years ago
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 → ---
(Assignee)

Updated

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

Comment 34

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

Comment 35

9 years ago
Created attachment 446811 [details] [diff] [review]
Patch (v1.1)

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)
(Assignee)

Comment 36

9 years ago
Created attachment 446863 [details] [diff] [review]
Patch (v1.2)

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?
(Assignee)

Comment 39

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

Comment 40

9 years ago
http://hg.mozilla.org/mozilla-central/rev/37b7ca43fafe
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Depends on: 568362
(Assignee)

Updated

8 years ago
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+
(Assignee)

Comment 42

8 years ago
I just realized that nsComputedDOMStyle::GetStyleContextForElement is not available on branches.  Dbaron, is there a replacement API which I can use?
(Assignee)

Comment 43

8 years ago
The branch function to be used is nsInspectorCSSUtils::GetStyleContextForContent.
(Assignee)

Comment 45

8 years ago
Created attachment 460585 [details] [diff] [review]
Regression fix for 1.9.2

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+
(Assignee)

Comment 47

8 years ago
Created attachment 460602 [details] [diff] [review]
Regression fix for 1.9.2

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+
(Assignee)

Comment 49

8 years ago
Bustage fix for 1.9.2 landed as:

<http://hg.mozilla.org/releases/mozilla-1.9.2/rev/177e3561f58c>
(Assignee)

Comment 50

8 years ago
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.
status1.9.1: .12-fixed → wontfix
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.
(Assignee)

Comment 52

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

Comment 54

8 years ago
(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!  :-)

Updated

8 years ago
Depends on: 586389

Updated

8 years ago
Attachment #446863 - Flags: approval1.9.1.12+ → approval1.9.1.12-
(Assignee)

Updated

8 years ago
Depends on: 595197
(Assignee)

Updated

8 years ago
Depends on: 596001
You need to log in before you can comment on or make changes to this bug.