Last Comment Bug 738385 - queryCommand*() should not throw for commands that don't support them
: queryCommand*() should not throw for commands that don't support them
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla14
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-22 11:55 PDT by :Aryeh Gregor (away until August 15)
Modified: 2012-04-18 05:13 PDT (History)
2 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (76.64 KB, patch)
2012-03-22 13:25 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Review
Patch part 1, v1 -- Refactor some editing logic in nsHTMLDocument.cpp (4.14 KB, patch)
2012-03-23 11:54 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Review
Patch part 2, v2 -- queryCommand*() should not throw for commands that don't support them (96.20 KB, patch)
2012-03-23 11:57 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Review
Patch part 1, v2 -- Refactor some editing logic in nsHTMLDocument.cpp (2.78 KB, patch)
2012-04-03 07:49 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Review
Patch part 1, v3 -- Refactor some editing logic in nsHTMLDocument.cpp (2.78 KB, patch)
2012-04-04 01:52 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Review
Patch part 2, v2.1 -- queryCommand*() should not throw for commands that don't support them (95.98 KB, patch)
2012-04-06 00:44 PDT, :Aryeh Gregor (away until August 15)
ayg: review+
Details | Diff | Review

Description :Aryeh Gregor (away until August 15) 2012-03-22 11:55:18 PDT
Spec:

http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#querycommandindeterm()
http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#querycommandstate()
http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#querycommandvalue()

For commands that don't have any special processing defined for indeterm/state/value, the spec says to return false/false/"" respectively.  E.g., document.queryCommandValue("bold"), or document.queryCommandIndeterm("stylewithcss").  This was discussed:

http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-September/033235.html

Originally the spec said to throw, which matches Gecko.  But Ryosuke had compat fears.  I think throwing in these cases makes sense, but realistically I think the path of least resistance is what the spec says now.  Asking IE/WebKit/Opera to all change to throwing when they didn't throw before has much bigger compat risk than Gecko changing to not throw.
Comment 1 :Ehsan Akhgari (busy, don't ask for review please) 2012-03-22 13:20:42 PDT
This sounds fine by me.  I doubt that there are too many web sites which rely on Gecko throwing in this case...  ;-)
Comment 2 :Aryeh Gregor (away until August 15) 2012-03-22 13:25:28 PDT
Created attachment 608444 [details] [diff] [review]
Patch v1

The code change here causes a lot of TEST-UNEXPECTED-PASS in browserscope, but no TEST-UNEXPECTED-FAIL (except the debug is()'s that dump the HTML).  The changes to currentStatus.js should serve as a good enough test of the functionality change.  The diff is bad, unfortunately, recording it as lots of additions followed by lots of deletions, because of how repetitive the data file is.

I'm not at all sure this is the best way to fix the code.
Comment 3 Mozilla RelEng Bot 2012-03-22 13:32:06 PDT
Autoland Patchset:
	Patches: 608444
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=37e27f74fefc
Try run started, revision 37e27f74fefc. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=37e27f74fefc
Comment 4 :Ehsan Akhgari (busy, don't ask for review please) 2012-03-22 16:15:44 PDT
Comment on attachment 608444 [details] [diff] [review]
Patch v1

Review of attachment 608444 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, assuming that you auto-generated currentStatus.js after your change.  :-)

(And yes, that would serve as enough testing.)
Comment 5 Mozilla RelEng Bot 2012-03-22 23:46:51 PDT
Try run for 37e27f74fefc is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=37e27f74fefc
Results (out of 220 total builds):
    success: 173
    warnings: 31
    failure: 16
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-37e27f74fefc
Comment 6 :Aryeh Gregor (away until August 15) 2012-03-23 11:54:57 PDT
Created attachment 608800 [details] [diff] [review]
Patch part 1, v1 -- Refactor some editing logic in nsHTMLDocument.cpp

When fixing the test_bug408231.html failures, I noticed that formatblock and heading were still throwing exceptions.  This was because QueryCommandIndeterm() and QueryCommandState() were both calling the version of ConvertToMidasInternalCommandInner that doesn't ignore parameters, and that version had a code path for cmd_paragraphState that returned false if the parameter wasn't recognized.  In this case the parameter was empty, so it wound up throwing NS_ERROR_NOT_IMPLEMENTED.  We only really wanted this logic for execCommand, so I moved it to ExecCommand.  While I was at it, I changed QueryCommandIndeterm to use the two-parameter version of ConvertToMidasInternalCommand, because it didn't need the extra params.

(This patch causes test failures in browserscope, which are fixed in the next patch.)
Comment 7 :Aryeh Gregor (away until August 15) 2012-03-23 11:57:45 PDT
Created attachment 608802 [details] [diff] [review]
Patch part 2, v2 -- queryCommand*() should not throw for commands that don't support them

This is the same patch as the v1 I submitted before.  The only difference is in test changes: I updated test_bug408231.html to account for this bug's patches, and updated currentStatus.js to fix more TEST-UNEXPECTED-PASS that resulted from the new patch part 1 in this series.  I also added indeterm tests to test_bug408231.html, and made it test for specific exception types rather than just generically testing for an exception to be thrown.
Comment 8 Mozilla RelEng Bot 2012-03-23 12:03:05 PDT
Autoland Patchset:
	Patches: 608800, 608802
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=915764a4b8f1
Try run started, revision 915764a4b8f1. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=915764a4b8f1
Comment 9 Mozilla RelEng Bot 2012-03-23 20:17:16 PDT
Try run for 915764a4b8f1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=915764a4b8f1
Results (out of 219 total builds):
    exception: 1
    success: 171
    warnings: 47
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-915764a4b8f1
Comment 10 :Aryeh Gregor (away until August 15) 2012-04-01 05:13:06 PDT
I don't think those crashtest failures are my fault, but let me double-check . . .
Comment 11 Mozilla RelEng Bot 2012-04-01 05:16:33 PDT
Autoland Patchset:
	Patches: 608800, 608802
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=b87aa6381e6e
Try run started, revision b87aa6381e6e. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=b87aa6381e6e
Comment 12 :Aryeh Gregor (away until August 15) 2012-04-01 05:25:02 PDT
Ehsan, it looks like this causes an unexpected pass for 636074-1.html.  Is it okay if I just update crashtests.list, i.e., do

--- a/editor/libeditor/base/crashtests/crashtests.list
+++ b/editor/libeditor/base/crashtests/crashtests.list
@@ -2,11 +2,11 @@ load 336104.html
 load 382527-1.html
 load 402172-1.html
 load 407079-1.html
 load 407256-1.html
 load 430624-1.html
 load 459613.html
 load 475132-1.xhtml
 asserts-if(!Android,1) load 633709.xhtml # Bug 695364
-asserts-if(!Android,6) load 636074-1.html # Bug 439258, charged to the wrong test due to bug 635550
+load 636074-1.html
 load 713427-1.html
 load 713427-2.xhtml

?  Based on reading the relevant bugs (the ones I have access to . . .), I doubt that this patch actually fixes the problem, so maybe it's just masking it, in which case probably the test should somehow be updated so that it still fails.  But I don't know what in my patches could have possibly caused the assertions to suddenly stop, so I don't know how to update the test.
Comment 13 Mozilla RelEng Bot 2012-04-01 08:17:10 PDT
Try run for b87aa6381e6e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b87aa6381e6e
Results (out of 13 total builds):
    success: 6
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-b87aa6381e6e
Comment 14 :Aryeh Gregor (away until August 15) 2012-04-02 07:58:10 PDT
Actually, this also seems to consistently cause

REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/docshell/base/crashtests/432114-2.html | load failed: timed out waiting for reftest-wait to be removed

That one probably *is* my fault, most likely due to the refactoring.  I'll look more closely at it.
Comment 15 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-02 10:32:49 PDT
(In reply to Aryeh Gregor from comment #12)
> Ehsan, it looks like this causes an unexpected pass for 636074-1.html.  Is
> it okay if I just update crashtests.list, i.e., do
> 
> --- a/editor/libeditor/base/crashtests/crashtests.list
> +++ b/editor/libeditor/base/crashtests/crashtests.list
> @@ -2,11 +2,11 @@ load 336104.html
>  load 382527-1.html
>  load 402172-1.html
>  load 407079-1.html
>  load 407256-1.html
>  load 430624-1.html
>  load 459613.html
>  load 475132-1.xhtml
>  asserts-if(!Android,1) load 633709.xhtml # Bug 695364
> -asserts-if(!Android,6) load 636074-1.html # Bug 439258, charged to the
> wrong test due to bug 635550
> +load 636074-1.html
>  load 713427-1.html
>  load 713427-2.xhtml
> 
> ?  Based on reading the relevant bugs (the ones I have access to . . .), I
> doubt that this patch actually fixes the problem, so maybe it's just masking
> it, in which case probably the test should somehow be updated so that it
> still fails.  But I don't know what in my patches could have possibly caused
> the assertions to suddenly stop, so I don't know how to update the test.

This assertion occurs in other tests as well, but I'm curious to know *why* your patch hides this.  I'm pretty sure that something in that test is now throwing...

(In reply to Aryeh Gregor from comment #14)
> Actually, this also seems to consistently cause
> 
> REFTEST TEST-UNEXPECTED-FAIL |
> file:///c:/talos-slave/test/build/reftest/tests/docshell/base/crashtests/
> 432114-2.html | load failed: timed out waiting for reftest-wait to be removed
> 
> That one probably *is* my fault, most likely due to the refactoring.  I'll
> look more closely at it.

Hmm, this one looks like something throwing as well.  I think there's probably a bug in your patch.
Comment 16 :Aryeh Gregor (away until August 15) 2012-04-03 07:49:43 PDT
Created attachment 611807 [details] [diff] [review]
Patch part 1, v2 -- Refactor some editing logic in nsHTMLDocument.cpp

It turns out that the problem that the moved check missed a condition that the original check had -- it was nested in if (gMidasCommandTable[i].useNewParam), but the moved code was being run even if that was false.  Now instead of ConvertToMidasInternalCommandInner returning false if it gets an invalid parameter for cmd_paragraphState, it truncates the output parameter.  ExecCommand then separately checks if the output parameter is empty and returns NS_OK if so.  This fixes the crashtest failures I was getting, and also means that QueryCommandState won't throw for formatBlock etc.
Comment 17 Mozilla RelEng Bot 2012-04-03 07:56:05 PDT
Autoland Patchset:
	Patches: 611807, 608802
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=e56453b68661
Try run started, revision e56453b68661. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=e56453b68661
Comment 18 Mozilla RelEng Bot 2012-04-03 12:47:20 PDT
Try run for e56453b68661 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e56453b68661
Results (out of 226 total builds):
    success: 168
    warnings: 57
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-e56453b68661
Comment 19 :Aryeh Gregor (away until August 15) 2012-04-04 01:52:05 PDT
Created attachment 612134 [details] [diff] [review]
Patch part 1, v3 -- Refactor some editing logic in nsHTMLDocument.cpp

Oops, dumb logic error in that patch.  I should have run tests in editor/ and content/html/document/ before submitting.  Now I have and it passes.  Interdiff:

diff --git a/content/html/document/src/nsHTMLDocument.cpp b/content/html/document/src/nsHTMLDocument.cpp
--- a/content/html/document/src/nsHTMLDocument.cpp
+++ b/content/html/document/src/nsHTMLDocument.cpp
@@ -2944,17 +2944,17 @@ ConvertToMidasInternalCommandInner(const
             for (j = 0; j < ArrayLength(gBlocks); ++j) {
               if (convertedParam.Equals(gBlocks[j],
                                         nsCaseInsensitiveCStringComparator())) {
                 outParam.Assign(gBlocks[j]);
                 break;
               }
             }
 
-            if (j != ArrayLength(gBlocks)) {
+            if (j == ArrayLength(gBlocks)) {
               outParam.Truncate();
             }
           }
           else {
             CopyUTF16toUTF8(inParam, outParam);
           }
         }
       }

Sorry for the review spam!
Comment 20 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-04 09:44:19 PDT
Comment on attachment 612134 [details] [diff] [review]
Patch part 1, v3 -- Refactor some editing logic in nsHTMLDocument.cpp

Review of attachment 612134 [details] [diff] [review]:
-----------------------------------------------------------------

Oops, should have caught that!
Comment 21 Mozilla RelEng Bot 2012-04-05 07:38:46 PDT
Autoland Patchset:
	Patches: 612134, 608802
	Branch: mozilla-central => try
Patch 608802 could not be applied to mozilla-central.
patching file content/html/content/test/test_bug408231.html
Hunk #1 FAILED at 52
Hunk #2 FAILED at 124
2 out of 3 hunks FAILED -- saving rejects to file content/html/content/test/test_bug408231.html.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.
Comment 22 :Aryeh Gregor (away until August 15) 2012-04-06 00:44:42 PDT
Created attachment 612813 [details] [diff] [review]
Patch part 2, v2.1 -- queryCommand*() should not throw for commands that don't support them

Resolve conflict with bug 738440.
Comment 23 Mozilla RelEng Bot 2012-04-09 07:05:52 PDT
Autoland Patchset:
	Patches: 612134, 612813
	Branch: mozilla-central => try
Patch 612813 could not be applied to mozilla-central.
patching file editor/libeditor/html/tests/browserscope/lib/richtext2/currentStatus.js
Hunk #2 FAILED at 9373
Hunk #9 FAILED at 23824
2 out of 9 hunks FAILED -- saving rejects to file editor/libeditor/html/tests/browserscope/lib/richtext2/currentStatus.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.
Comment 24 :Aryeh Gregor (away until August 15) 2012-04-15 00:22:55 PDT
New try run (together with bug 279330): https://tbpl.mozilla.org/?tree=Try&rev=863b4acac1d4
Comment 25 :Aryeh Gregor (away until August 15) 2012-04-15 04:20:09 PDT
Try run passed; failures are due only to bug 279330.  See <https://tbpl.mozilla.org/?tree=Try&rev=23b3a61d4ca3> and <https://tbpl.mozilla.org/?tree=Try&rev=0a582f0d148c> for evidence to that effect.  Will land with bug 279330.

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