Last Comment Bug 725069 - Html Comments being shown in DOM as text nodes. Regression from firefox 9.0
: Html Comments being shown in DOM as text nodes. Regression from firefox 9.0
Status: RESOLVED FIXED
[qa+][qa!:11][qa!:12][qa!:13]
: regression
Product: Core
Classification: Components
Component: Editor (show other bugs)
: 10 Branch
: x86_64 Windows 7
: -- normal (vote)
: mozilla13
Assigned To: :Ehsan Akhgari
:
:
Mentors:
: 726431 (view as bug list)
Depends on:
Blocks: 688789
  Show dependency treegraph
 
Reported: 2012-02-07 13:20 PST by Tom Hindle
Modified: 2012-05-08 08:58 PDT (History)
12 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
affected
+
verified
+
verified
+
verified
-
affected


Attachments
test case. (288 bytes, text/html)
2012-02-07 13:22 PST, Tom Hindle
no flags Details
Patch (v1) (3.50 KB, patch)
2012-02-07 16:09 PST, :Ehsan Akhgari
roc: review+
akeybl: approval‑mozilla‑release-
Details | Diff | Splinter Review
Follow-up (1.49 KB, patch)
2012-02-08 13:39 PST, :Ehsan Akhgari
roc: review+
Details | Diff | Splinter Review
Rolled up patch for branches (3.85 KB, patch)
2012-02-08 15:19 PST, :Ehsan Akhgari
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Tom Hindle 2012-02-07 13:20:16 PST
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/535.7 (KHTML, like Gecko) Chrome/16.0.912.77 Safari/535.7

Steps to reproduce:

Example test page that alerts the body's firstChild value node, and alerts the body's second child.

<html>
<head>
<title></title>
<script>
function outputDocument() {
alert(document.body.firstChild.nodeValue);
alert(document.body.firstChild.nextSibling);
};
</script>
</head>
<body contenteditable="true" onload="outputDocument()">abc<!--helloworld-->bcd</body>
</html>


Actual results:

displays 'abchelloworldbcd' , null




Expected results:

should display 'abc', [object comment]
Comment 1 Tom Hindle 2012-02-07 13:22:16 PST
Created attachment 595146 [details]
test case.
Comment 2 Tom Hindle 2012-02-07 13:27:04 PST
Only occurs when contenteditable=true blocks.
Comment 3 Alice0775 White 2012-02-07 13:45:56 PST
Regression window(m-c)
Works:
http://hg.mozilla.org/mozilla-central/rev/9fa62f76f1cf
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111019 Firefox/10.0a1 ID:20111019031031
Fails:
http://hg.mozilla.org/mozilla-central/rev/311fdb9b38b7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111020 Firefox/10.0a1 ID:20111020031025
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9fa62f76f1cf&tochange=311fdb9b38b7


Regression window(m-i)
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/a86a80a91234
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111018 Firefox/10.0a1 ID:20111018125055
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/cb1c23bd837b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111018 Firefox/10.0a1 ID:20111018140655
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a86a80a91234&tochange=cb1c23bd837b

Suspected:
7ec3daabbf47	Ehsan Akhgari — Bug 688789 - Stop touching the frame tree to determine whether a node is editable or not; r=roc
Comment 4 :Ehsan Akhgari 2012-02-07 16:09:13 PST
Created attachment 595239 [details] [diff] [review]
Patch (v1)
Comment 5 :Ehsan Akhgari 2012-02-07 16:09:38 PST
http://tbpl.mozilla.org/?tree=Try&rev=8bbbccba56b7
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-07 16:11:22 PST
Better get this onto branches...
Comment 7 Alex Keybl [:akeybl] 2012-02-07 17:01:36 PST
Tracking for FF11 and up. We are going to build with our 10.0.1 chemspill tonight, so if there's any reason to think that this should be included, please email release-drivers ASAP.

To my knowledge, we don't know of any major website regressions caused by this regression. From reading this bug, my understanding is that any site running into this issue could change the HTML on their end.
Comment 8 :Ehsan Akhgari 2012-02-07 19:23:03 PST
Comment on attachment 595239 [details] [diff] [review]
Patch (v1)

[Approval Request Comment]
Regression caused by (bug #): 688789
User impact if declined: HTML comments might show up on websites.
Testing completed (on m-c, etc.): Landing on m-c right now, tested locally.
Risk to taking this patch (and alternatives if risky): It's extremely low risk.  The change is well understood.
Comment 10 :Ehsan Akhgari 2012-02-07 19:26:02 PST
Emailed release-drivers about this.
Comment 11 Alex Keybl [:akeybl] 2012-02-07 19:46:26 PST
Comment on attachment 595239 [details] [diff] [review]
Patch (v1)

[Triage Comment]
If this patch had baked on m-c for at least a couple of days prior to our go-to-build, I would be more in favor of taking it.  Our bar for taking fixes on mozilla-release is typically low risk and well tested, with high reward. In this case, we haven't gotten any related reports from major sites, only contenteditable blocks with HTML comments inside are affected, the site can make a change if necessary, and we haven't gotten any regression testing around this yet. I'd consider that non-zero risk with little testing and low reward.

Approving for mozilla-aurora and mozilla-beta, however.
Comment 12 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2012-02-07 23:08:00 PST
Why was tracking-esr10 minused? Seems like a very bad situation to have a bug of this level of brokenness in ESR for the whole cycle. Especially when the fix looks super-safe even for a chemspill.
Comment 13 Mozilla RelEng Bot 2012-02-08 03:00:28 PST
Try run for 8bbbccba56b7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8bbbccba56b7
Results (out of 210 total builds):
    exception: 1
    success: 186
    warnings: 21
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-8bbbccba56b7
 Timed out after 06 hours without completing.
Comment 14 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-02-08 07:51:24 PST
Comment on attachment 595239 [details] [diff] [review]
Patch (v1)


>diff --git a/editor/libeditor/base/nsEditor.cpp b/editor/libeditor/base/nsEditor.cpp
>--- a/editor/libeditor/base/nsEditor.cpp
>+++ b/editor/libeditor/base/nsEditor.cpp
>@@ -3631,16 +3631,18 @@ nsEditor::IsEditable(nsIContent *aNode)
>   // see if it has a frame.  If so, we'll edit it.
>   // special case for textnodes: frame must have width.
>   if (aNode->IsElement() && !IsElementVisible(aNode->AsElement())) {
>     // If the element has no frame, it's not editable.  Note that we
>     // need to check IsElement() here, because some of our tests
>     // rely on frameless textnodes being visible.
>     return false;
>   }
>+  if (aNode->NodeType() == nsIDOMNode::COMMENT_NODE)
>+    return false;
>   if (aNode->NodeType() != nsIDOMNode::TEXT_NODE)
>     return true;  // not a text node; not invisible

What about PI? Should it be handled like comment node?
Or do we never handle PI nodes here?
Comment 15 Ed Morley [:emorley] 2012-02-08 09:00:48 PST
https://hg.mozilla.org/mozilla-central/rev/3e246f6074a1
Comment 16 :Ehsan Akhgari 2012-02-08 13:08:14 PST
(In reply to Olli Pettay [:smaug] from comment #14)
> Comment on attachment 595239 [details] [diff] [review]
> Patch (v1)
> 
> 
> >diff --git a/editor/libeditor/base/nsEditor.cpp b/editor/libeditor/base/nsEditor.cpp
> >--- a/editor/libeditor/base/nsEditor.cpp
> >+++ b/editor/libeditor/base/nsEditor.cpp
> >@@ -3631,16 +3631,18 @@ nsEditor::IsEditable(nsIContent *aNode)
> >   // see if it has a frame.  If so, we'll edit it.
> >   // special case for textnodes: frame must have width.
> >   if (aNode->IsElement() && !IsElementVisible(aNode->AsElement())) {
> >     // If the element has no frame, it's not editable.  Note that we
> >     // need to check IsElement() here, because some of our tests
> >     // rely on frameless textnodes being visible.
> >     return false;
> >   }
> >+  if (aNode->NodeType() == nsIDOMNode::COMMENT_NODE)
> >+    return false;
> >   if (aNode->NodeType() != nsIDOMNode::TEXT_NODE)
> >     return true;  // not a text node; not invisible
> 
> What about PI? Should it be handled like comment node?
> Or do we never handle PI nodes here?

Not sure... Maybe it's best to code defensively and only handle things which we knwow how to handle?  Patch coming up.
Comment 17 :Ehsan Akhgari 2012-02-08 13:39:06 PST
Created attachment 595515 [details] [diff] [review]
Follow-up
Comment 18 :Ehsan Akhgari 2012-02-08 15:10:36 PST
Follow-up: https://hg.mozilla.org/integration/mozilla-inbound/rev/319914797e70
Comment 19 :Ehsan Akhgari 2012-02-08 15:19:56 PST
Created attachment 595552 [details] [diff] [review]
Rolled up patch for branches

This patch incorporates the follow-up patch to the one which is already approved on this bug.
Comment 20 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-02-09 03:27:57 PST
Comment on attachment 595552 [details] [diff] [review]
Rolled up patch for branches


>-  if (aNode->NodeType() != nsIDOMNode::TEXT_NODE)
>-    return true;  // not a text node; not invisible
>-
>-  return IsTextInDirtyFrameVisible(aNode);
>+  switch (aNode->NodeType()) {
>+    case nsIDOMNode::ELEMENT_NODE:
>+      return true; // not a text node; not invisible
>+    case nsIDOMNode::TEXT_NODE:
>+      return IsTextInDirtyFrameVisible(aNode);
>+    default:
>+      return false;
>+  }
> }

What about CDATA_SECTION?
Comment 21 Ed Morley [:emorley] 2012-02-09 10:19:22 PST
https://hg.mozilla.org/mozilla-central/rev/319914797e70

Leaving open for comment 20.
Comment 22 :Ehsan Akhgari 2012-02-09 12:38:02 PST
(In reply to Olli Pettay [:smaug] from comment #20)
> Comment on attachment 595552 [details] [diff] [review]
> Rolled up patch for branches
> 
> 
> >-  if (aNode->NodeType() != nsIDOMNode::TEXT_NODE)
> >-    return true;  // not a text node; not invisible
> >-
> >-  return IsTextInDirtyFrameVisible(aNode);
> >+  switch (aNode->NodeType()) {
> >+    case nsIDOMNode::ELEMENT_NODE:
> >+      return true; // not a text node; not invisible
> >+    case nsIDOMNode::TEXT_NODE:
> >+      return IsTextInDirtyFrameVisible(aNode);
> >+    default:
> >+      return false;
> >+  }
> > }
> 
> What about CDATA_SECTION?

I don't think that we need to handle those.  The parser will convert them to textnodes, right?
Comment 23 Alex Keybl [:akeybl] 2012-02-09 13:41:14 PST
Comment on attachment 595552 [details] [diff] [review]
Rolled up patch for branches

[Triage Comment]
Deemed low risk and prevents comments from incorrectly being displayed - approving for Aurora/Beta.
Comment 24 Alex Keybl [:akeybl] 2012-02-09 13:43:57 PST
(In reply to Henri Sivonen (:hsivonen) from comment #12)
> Why was tracking-esr10 minused? Seems like a very bad situation to have a
> bug of this level of brokenness in ESR for the whole cycle. Especially when
> the fix looks super-safe even for a chemspill.

This bug doesn't meet the criteria as outlined in [1] (under Assumptions) and [2], and hasn't been brought to our attention as a major pain point by enterprise users. We're being strict with our accepted changes to prevent the floodgates from opening on a product with minimal beta test support.

[1] https://wiki.mozilla.org/Enterprise/Firefox/ExtendedSupport:Proposal#Assumptions
[2] https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
Comment 25 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-02-09 14:01:54 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #22)
> > What about CDATA_SECTION?
> 
> I don't think that we need to handle those.  The parser will convert them to
> textnodes, right?
In XHTML? Or is this code only for HTML?
Comment 26 :Ehsan Akhgari 2012-02-09 18:28:55 PST
(In reply to Olli Pettay [:smaug] from comment #25)
> (In reply to Ehsan Akhgari [:ehsan] from comment #22)
> > > What about CDATA_SECTION?
> > 
> > I don't think that we need to handle those.  The parser will convert them to
> > textnodes, right?
> In XHTML? Or is this code only for HTML?

HTML.  Our editor is pretty broken on XHTML as it's making tons of different assumptions which won't hold in XHTML documents.
Comment 28 Cacycle 2012-02-12 08:03:36 PST
(In reply to Alex Keybl [:akeybl] from comment #11)
> Comment on attachment 595239 [details] [diff] [review]
> Patch (v1)
> 
> [Triage Comment]
> If this patch had baked on m-c for at least a couple of days prior to our
> go-to-build, I would be more in favor of taking it.  Our bar for taking
> fixes on mozilla-release is typically low risk and well tested, with high
> reward. In this case, we haven't gotten any related reports from major
> sites, only contenteditable blocks with HTML comments inside are affected,
> the site can make a change if necessary, and we haven't gotten any
> regression testing around this yet. I'd consider that non-zero risk with
> little testing and low reward.
> 
> Approving for mozilla-aurora and mozilla-beta, however.

This bug affects all rich text editors that are v(In reply to Alex Keybl [:akeybl] from comment #7)
> Tracking for FF11 and up. We are going to build with our 10.0.1 chemspill
> tonight, so if there's any reason to think that this should be included,
> please email release-drivers ASAP.
> 
> To my knowledge, we don't know of any major website regressions caused by
> this regression. From reading this bug, my understanding is that any site
> running into this issue could change the HTML on their end.

This is probably a duplicate of bug 726431. That bug potentially affects all rich texts editors. These editors are widely used for editing existing data and text via web frontends. This bug does introduce errors into existing data (e.g. when editing Wikipedia and other wiki texts with wikEd). Also, there is no way for a site to fix this issue server-side. Therefore, this bug has a very high risk and an immediate fix in Firefox 10 would be of high importance.
Comment 29 Alex Keybl [:akeybl] 2012-02-13 18:02:54 PST
(In reply to Cacycle from comment #28)
> This is probably a duplicate of bug 726431. That bug potentially affects all
> rich texts editors. These editors are widely used for editing existing data
> and text via web frontends. This bug does introduce errors into existing
> data (e.g. when editing Wikipedia and other wiki texts with wikEd). Also,
> there is no way for a site to fix this issue server-side. Therefore, this
> bug has a very high risk and an immediate fix in Firefox 10 would be of high
> importance.

If/when this is confirmed as a significant pain point for the FF10 ESR, we'll consider landing on that branch. For non-security issues, that's the bar we've set based upon the testing coverage we receive prior to release.
Comment 30 Paul Silaghi, QA [:pauly] 2012-02-24 02:51:13 PST
Actual results:
'abc', [object comment]

Verified fixed on Firefox 11b4:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (X11; Linux i686; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0) Gecko/20100101 Firefox/11.0
Comment 31 Paul Silaghi, QA [:pauly] 2012-03-20 07:03:30 PDT
Verified fixed on Firefox 12b1:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0) Gecko/20100101 Firefox/12.0
Comment 32 :Ehsan Akhgari 2012-04-23 19:06:36 PDT
*** Bug 726431 has been marked as a duplicate of this bug. ***
Comment 33 Paul Silaghi, QA [:pauly] 2012-05-08 08:58:58 PDT
Verified fixed on Firefox 13b2:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20100101 Firefox/13.0

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