Last Comment Bug 756045 - Turn "ASSERTION: anonymous nodes should not be in child lists" into an NS_WARNING
: Turn "ASSERTION: anonymous nodes should not be in child lists" into an NS_WAR...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla15
Assigned To: :Aryeh Gregor (working until September 2)
:
Mentors:
Depends on:
Blocks: 439258 751842
  Show dependency treegraph
 
Reported: 2012-05-17 02:48 PDT by :Aryeh Gregor (working until September 2)
Modified: 2012-05-18 11:04 PDT (History)
3 users (show)
ayg: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (1.84 KB, patch)
2012-05-17 03:11 PDT, :Aryeh Gregor (working until September 2)
no flags Details | Diff | Splinter Review
Patch v2, try failure fixed (2.48 KB, patch)
2012-05-17 06:49 PDT, :Aryeh Gregor (working until September 2)
bzbarsky: review+
Details | Diff | Splinter Review

Description :Aryeh Gregor (working until September 2) 2012-05-17 02:48:28 PDT
See bug 751842 comment 32.  We know about the assertion, and we aren't going to fix it anytime soon.  It gets hit a million time during editor tests, which both slows them down and clogs the logs.
Comment 1 :Aryeh Gregor (working until September 2) 2012-05-17 03:11:54 PDT
Created attachment 624687 [details] [diff] [review]
Patch v1

This might need more test updates.
Comment 2 :Aryeh Gregor (working until September 2) 2012-05-17 04:10:02 PDT
Bah, autoland is flaky.  Manual try push: https://tbpl.mozilla.org/?tree=Try&rev=eb9a4fff4e2f
Comment 3 :Aryeh Gregor (working until September 2) 2012-05-17 06:49:18 PDT
Created attachment 624724 [details] [diff] [review]
Patch v2, try failure fixed
Comment 4 Boris Zbarsky [:bz] 2012-05-17 07:33:05 PDT
Why are editor tests hitting this?  The manual insanity it does with the resize handles or something?
Comment 5 :Aryeh Gregor (working until September 2) 2012-05-17 08:12:38 PDT
I'll leave Ehsan to answer that . . .
Comment 6 :Ehsan Akhgari 2012-05-17 08:20:46 PDT
(In reply to Boris Zbarsky (:bz) from comment #4)
> Why are editor tests hitting this?  The manual insanity it does with the
> resize handles or something?

There's a ton of cases where you can hit this from content script.  Calling .focus() on an abs-pos editable element is one example.
Comment 7 Boris Zbarsky [:bz] 2012-05-17 08:25:47 PDT
> Calling .focus() on an abs-pos editable element is one example.

Er.... why does that hit this assert?

Basically, if hitting this case is nominally a bug, we should document here why this is a warning, not an assert, and document at the buggy callsites that once they're fixed this should become an assert.

If this case is not nominally a bug, we shouldn't have a warning.
Comment 8 :Ehsan Akhgari 2012-05-17 10:41:02 PDT
(In reply to Boris Zbarsky (:bz) from comment #7)
> > Calling .focus() on an abs-pos editable element is one example.
> 
> Er.... why does that hit this assert?

Because that triggers the resizing and positioning controls to be displayed on focus, and we hit this assertion once for every one of those controls.

> Basically, if hitting this case is nominally a bug, we should document here
> why this is a warning, not an assert, and document at the buggy callsites
> that once they're fixed this should become an assert.
> 
> If this case is not nominally a bug, we shouldn't have a warning.

The fact that we hit this assert is because we inject those controls in a very bad way, but that bad way works as expected, so this is technically not a bug.  It's just ugly things that we should not be doing.  The proper way of doing this is to add those as anonymous abs-pos children of the element they're supposed to control, but doing that requires us to enable all sorts of frames to be abs-pos containers, and also make sure that elements such as images can deal with anonymous children, so it is a longer term project.
Comment 9 Boris Zbarsky [:bz] 2012-05-17 16:38:29 PDT
Oh, I'd somehow misread "editable element" as "text input".  I have _no_ idea why, and I'm sorry about the confusion.

So yes, this is all about the resize handles.

Aryeh, please add comments to the code where the warning is pointing that out, and please add code comments to the caller of ContentRemoved caller in editor about reinstating the assert once that caller is fixed.
Comment 10 Boris Zbarsky [:bz] 2012-05-17 20:31:03 PDT
Comment on attachment 624724 [details] [diff] [review]
Patch v2, try failure fixed

r=me with my requested comment fixes.
Comment 11 :Aryeh Gregor (working until September 2) 2012-05-17 23:23:23 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/8be8a188bd60
Comment 12 Ed Morley [:emorley] 2012-05-18 11:04:36 PDT
https://hg.mozilla.org/mozilla-central/rev/8be8a188bd60

Note You need to log in before you can comment on or make changes to this bug.