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 (:ayg) (next working March 28-April 26)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 439258 751842
  Show dependency treegraph
 
Reported: 2012-05-17 02:48 PDT by Aryeh Gregor (:ayg) (next working March 28-April 26)
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 (:ayg) (next working March 28-April 26)
no flags Details | Diff | Splinter Review
Patch v2, try failure fixed (2.48 KB, patch)
2012-05-17 06:49 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
bzbarsky: review+
Details | Diff | Splinter Review

Description Aryeh Gregor (:ayg) (next working March 28-April 26) 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 (:ayg) (next working March 28-April 26) 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 (:ayg) (next working March 28-April 26) 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 (:ayg) (next working March 28-April 26) 2012-05-17 06:49:18 PDT
Created attachment 624724 [details] [diff] [review]
Patch v2, try failure fixed
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 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 (:ayg) (next working March 28-April 26) 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] (still a bit busy) 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] (still a bit busy) 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] (still a bit busy) 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 (:ayg) (next working March 28-April 26) 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.