336104 - "ASSERTION: no frame, see bug #188946"

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

###!!! 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.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

Attachment: 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.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 4

[(no longer active)]

Investigating.  I think the second solution is the way to go here (and we also need to remove the assertion for sure!).

Comment 5

[(no longer active)]

Attachment: 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?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So this is going to flush right? Are the editor callers of this method prepared to handle frame destruction?

Comment 7

[(no longer active)]

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

Comment 8

[(no longer active)]

(This is especially risky because frame destruction could lead to the editor itself being destructed as well.)

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe something higher up the stack should flush and check for the existence of a frame?

Comment 10

[(no longer active)]

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.

Comment 11

[(no longer active)]

Attachment: Patch (v2)

Use the "noflush" variant.  This passes a full try unit test run.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 13

[(no longer active)]

dbaron: please see comment 12.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 17

[(no longer active)]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

No, but why would such a thing help you?

Comment 19

[(no longer active)]

It should make sure that we have a style context based on the style changes happened so far, doesn't it?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Yes, but why is the "without doing a content flush" part useful?

Comment 21

[(no longer active)]

See comment 6 and 7.

Comment 22

[Boris Zbarsky [:bzbarsky]]

But style change processing on its own can run arbitrary script; hence my questions...

Comment 23

[(no longer active)]

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.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 25

[(no longer active)]

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

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Editor mostly works with DOM nodes, not frames. I think we should be able to flush in most places inside editor.

Comment 27

[(no longer active)]

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

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Generally, if the callers aren't holding pointers to frames then they are able to handle frame destruction.

Comment 29

[(no longer active)]

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

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 443509 [details] [diff] [review]
Patch (v1)

ok

Comment 31

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/0d2f650b8581

Comment 32

[(no longer active)]

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...

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 34

[(no longer active)]

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.

Comment 35

[(no longer active)]

Attachment: 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.

Comment 36

[(no longer active)]

Attachment: Patch (v1.2)

Removed two more assertions which were fixed from the manifests.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 446863 [details] [diff] [review]
Patch (v1.2)

r=dbaron on the differences from the last patch

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 39

[(no longer active)]

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

Comment 40

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/37b7ca43fafe

Comment 41

[Daniel Veditz [:dveditz]]

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

Comment 42

[(no longer active)]

I just realized that nsComputedDOMStyle::GetStyleContextForElement is not available on branches.  Dbaron, is there a replacement API which I can use?

Comment 43

[(no longer active)]

The branch function to be used is nsInspectorCSSUtils::GetStyleContextForContent.

Comment 44

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/89a4a52baa86
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/42be3232515d

Comment 45

[(no longer active)]

Attachment: 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.

Comment 46

[Johnny Stenback (:jst)]

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.

Comment 47

[(no longer active)]

Attachment: Regression fix for 1.9.2

With comments addressed.

Comment 48

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 460602 [details] [diff] [review]
Regression fix for 1.9.2

a=beltzner

Comment 49

[(no longer active)]

Bustage fix for 1.9.2 landed as:

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

Comment 50

[(no longer active)]

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.

Comment 51

[Mark Banner (:standard8)]

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.

Comment 52

[(no longer active)]

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

Comment 53

[Mark Banner (:standard8)]

(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

Comment 54

[(no longer active)]

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