Closed Bug 1408227 Opened 7 years ago Closed 7 years ago

Correctly update the child at offset pointer across calls to WSRunObject::InsertBreak()

Categories

(Core :: DOM: Editor, defect, P1)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- unaffected
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: masayuki)

References

Details

Attachments

(6 files, 1 obsolete file)

Bug 1408227 - part 1: TextEditor::CreateBRImpl() should take |const EditorRawDOMPoint&| for insertion point of new <br> element
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1408227 - part 2: WSRunObject::InsertBreak() should take |const EditorRawDOMPoint&| as an argument
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1408227 - part 3: Redesign WSRunObject::FindRun() with EditorRawDOMPoint
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1408227 - part 4: WSRunObject::DeleteChars() should take two |const EditorRawDOMPoint&| arguments to specify a range to remove
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1408227 - part 5: Redesign GetCharAfter(), GetCharBefore(), GetWSPointAfter() and GetWSPointBefore() of WSRunObject
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1408227 - part 6: WSRunObject::CheckTrailingNBSP() should take |const EditorRawDOMPoint&| instead of a set of container node and offset in it
59 bytes, text/x-review-board-request
m_kato
: review+
Details 
      No description provided.
Assignee: nobody → ehsan
Blocks: 1406482
This patch fixes the problem I talked about in bug 1407966 comment 9.
Keywords: regression
Comment on attachment 8918064 [details] [diff] [review]
Correctly update the child at offset pointer across calls to WSRunObject::InsertBreak()

> nsresult
> TextEditor::CreateBRImpl(nsCOMPtr<nsIDOMNode>* aInOutParent,
>                          int32_t* aInOutOffset,
>+                         nsCOMPtr<nsIContent>* aInOutChildAtOffset,
>                          nsCOMPtr<nsIDOMNode>* outBRNode,
>                          EDirection aSelect)
> {
>   NS_ENSURE_TRUE(aInOutParent && *aInOutParent && aInOutOffset && outBRNode, NS_ERROR_NULL_POINTER);
>   *outBRNode = nullptr;
> 
>   // we need to insert a br.  unfortunately, we may have to split a text node to do it.
>   nsCOMPtr<nsINode> node = do_QueryInterface(*aInOutParent);
>   int32_t theOffset = *aInOutOffset;
>+  int32_t offsetChange = 1;

Really difficult to understand what this 1 means. Please set 0 to here since and set it when we change *aInOutChildAtOffset.

Or, I like this way is better, declare |const int32_t kInOffset = *aInOutOffset;|. Then, you can check the difference with |*aInOutChildAtOffset - kInOffset| at the new for loop.

>@@ -460,24 +464,30 @@ TextEditor::CreateBRImpl(nsCOMPtr<nsIDOMNode>* aInOutParent,
>       tmp = GetNodeLocation(node, &offset);
>     }
>     // create br
>     brNode = CreateNode(nsGkAtoms::br, tmp, offset);
>     if (NS_WARN_IF(!brNode)) {
>       return NS_ERROR_FAILURE;
>     }
>     *aInOutParent = GetAsDOMNode(tmp);
>+    offsetChange = offset + 2 - *aInOutOffset; // (end - start) + 1

With your comment, I understand this as:

(OutOffset - InOffset) + 1.

So, I like |(offset + 1) - *aInOutOffset + 1|.

However, I don't understand the last |+ 1|. Could you explain it with comment? Or please consider to use kInOffset.

>     *aInOutOffset = offset+1;
>   } else {
>     brNode = CreateNode(nsGkAtoms::br, node, theOffset);
>     if (NS_WARN_IF(!brNode)) {
>       return NS_ERROR_FAILURE;
>     }
>     (*aInOutOffset)++;

Please set offsetChange to 1 here or do nothing if you use kInOffset.

>+  for (int32_t i = 0; i < offsetChange; ++i) {

So, I guess this can be:

  for (int32_t i = 0; i < *aInOutChildAtOffset - kInOffset; ++i) {

Otherwise, looks okay to me. Although, I'd like to check the new patch in usual schedule. However, I'll go away starting tomorrow. So, giving r+ for making you can land new patch.
Attachment #8918064 - Flags: review?(masayuki) → review+
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/febed94fa592
Correctly update the child at offset pointer across calls to WSRunObject::InsertBreak(); r=masayuki
Backed out for asserting at editor/libeditor/EditorBase.cpp:2513 during execution of web-platform-test /editing/run/inserttext.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/763378dbc0035a1cbe81270d16a088e9d3569ca6

Push which ran failing test: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=508179b0dad5311979a135b9a03721da53ca868b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=136827503&repo=mozilla-inbound

[task 2017-10-13T15:28:07.400Z] 15:28:07     INFO - TEST-START | /editing/run/inserttext.html
[task 2017-10-13T15:28:07.456Z] 15:28:07     INFO - PID 3041 | [Child 3091, Main Thread] WARNING: NS_ENSURE_TRUE(mDoneSetup) failed: file /builds/worker/workspace/build/src/editor/composer/nsEditingSession.cpp, line 1284
[task 2017-10-13T15:28:07.513Z] 15:28:07     INFO - PID 3041 | ++DOCSHELL 0x7fba40319000 == 4 [pid = 3091] [id = {991651aa-3d88-4ee9-8419-2dac3f4fa960}]
[task 2017-10-13T15:28:07.514Z] 15:28:07     INFO - PID 3041 | ++DOMWINDOW == 10 (0x7fba40319800) [pid = 3091] [serial = 65] [outer = (nil)]
[task 2017-10-13T15:28:07.529Z] 15:28:07     INFO - PID 3041 | [Parent 3041, Main Thread] WARNING: [nsFrameLoader] ReallyStartLoadingInternal tried but couldn't show remote browser.
[task 2017-10-13T15:28:07.530Z] 15:28:07     INFO - PID 3041 | : file /builds/worker/workspace/build/src/dom/base/nsFrameLoader.cpp, line 904
[task 2017-10-13T15:28:07.538Z] 15:28:07     INFO - PID 3041 | ++DOMWINDOW == 11 (0x7fba4031a800) [pid = 3091] [serial = 66] [outer = 0x7fba40319800]
[task 2017-10-13T15:28:07.607Z] 15:28:07     INFO - PID 3041 | 1507908487600	Marionette	DEBUG	Register listener.js for window 2147483713
[task 2017-10-13T15:28:07.689Z] 15:28:07     INFO - PID 3041 | ++DOMWINDOW == 12 (0x7fba405ab000) [pid = 3091] [serial = 67] [outer = 0x7fba40319800]
[task 2017-10-13T15:28:07.788Z] 15:28:07     INFO - PID 3041 | [Child 3091, Main Thread] WARNING: stylo: Web Components not supported yet: file /builds/worker/workspace/build/src/dom/base/nsDocument.cpp, line 6457
[task 2017-10-13T15:28:07.788Z] 15:28:07     INFO - PID 3041 | [Child 3091, Main Thread] WARNING: stylo: Web Components not supported yet: file /builds/worker/workspace/build/src/dom/base/nsDocument.cpp, line 6457
[task 2017-10-13T15:28:08.111Z] 15:28:08     INFO - PID 3041 | Assertion failure: node->GetChildAt(offset) == *aInOutChildAtOffset, at /builds/worker/workspace/build/src/editor/libeditor/EditorBase.cpp:2513
[task 2017-10-13T15:28:08.329Z] 15:28:08     INFO - PID 3041 | #01: mozilla::WSRunObject::InsertText(nsTSubstring<char16_t> const&, nsCOMPtr<nsINode>*, nsCOMPtr<nsIContent>*, int*, nsIDocument*) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.333Z] 15:28:08     INFO - PID 3041 | #02: mozilla::HTMLEditRules::WillInsertText(EditAction, mozilla::dom::Selection*, bool*, bool*, nsTSubstring<char16_t> const*, nsTSubstring<char16_t>*, int) [clone .part.427] (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.342Z] 15:28:08     INFO - PID 3041 | #03: mozilla::HTMLEditRules::WillDoAction(mozilla::dom::Selection*, mozilla::RulesInfo*, bool*, bool*) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.351Z] 15:28:08     INFO - PID 3041 | #04: mozilla::TextEditor::InsertText(nsTSubstring<char16_t> const&) [clone .part.180] (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.359Z] 15:28:08     INFO - PID 3041 | #05: mozilla::InsertPlaintextCommand::DoCommandParams(char const*, nsICommandParams*, nsISupports*) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.363Z] 15:28:08     INFO - PID 3041 | #06: nsControllerCommandTable::DoCommandParams(char const*, nsICommandParams*, nsISupports*) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.372Z] 15:28:08     INFO - PID 3041 | #07: nsBaseCommandController::DoCommandWithParams(char const*, nsICommandParams*) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.380Z] 15:28:08     INFO - PID 3041 | #08: nsCommandManager::DoCommand(char const*, nsICommandParams*, mozIDOMWindowProxy*) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.384Z] 15:28:08     INFO - PID 3041 | #09: nsHTMLDocument::ExecCommand(nsTSubstring<char16_t> const&, bool, nsTSubstring<char16_t> const&, nsIPrincipal&, mozilla::ErrorResult&) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.393Z] 15:28:08     INFO - PID 3041 | #10: mozilla::dom::HTMLDocumentBinding::execCommand(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.397Z] 15:28:08     INFO - PID 3041 | #11: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.405Z] 15:28:08     INFO - PID 3041 | #12: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.409Z] 15:28:08     INFO - PID 3041 | #13: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.417Z] 15:28:08     INFO - PID 3041 | #14: InternalCall(JSContext*, js::AnyInvokeArgs const&) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.422Z] 15:28:08     INFO - PID 3041 | #15: Interpret(JSContext*, js::RunState&) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.430Z] 15:28:08     INFO - PID 3041 | #16: js::RunScript(JSContext*, js::RunState&) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.434Z] 15:28:08     INFO - PID 3041 | #17: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.435Z] 15:28:08     INFO - PID 3041 | #18: InternalCall(JSContext*, js::AnyInvokeArgs const&) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.443Z] 15:28:08     INFO - PID 3041 | #19: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.451Z] 15:28:08     INFO - PID 3041 | #20: js::fun_apply(JSContext*, unsigned int, JS::Value*) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.451Z] 15:28:08     INFO - PID 3041 | #21: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.452Z] 15:28:08     INFO - PID 3041 | #22: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.452Z] 15:28:08     INFO - PID 3041 | #23: InternalCall(JSContext*, js::AnyInvokeArgs const&) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.456Z] 15:28:08     INFO - PID 3041 | #24: js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (/builds/worker/workspace/build/application/firefox/libxul.so)
[task 2017-10-13T15:28:08.456Z] 15:28:08     INFO - PID 3041 | #25: ??? (???:???)
This bug needs an owner to debug the test failure and reland this...
Note that Assertion failure: node->GetChildAt(offset) == *aInOutChildAtOffset is the summary of bug 1407966 from which we got here. It's all caused by bug 1406482 whose backout I've requested a few times.
Assignee: ehsan → catalin.badea392
Priority: -- → P1
Just out of interest: How important is this bug? It's a regression of bug 1406482 but what are the symptoms now that bug 1407966 is fixed?
Flags: needinfo?(masayuki)
Flags: needinfo?(catalin.badea392)
See Also: → 1409520
Currently (with patches under review), editor code is always validated or computes offset properly. So, this must not be a regression for now but we need to redesign Insert*() methods with Editor(Raw)DOMPoint.
Assignee: catalin.badea392 → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)
Flags: needinfo?(catalin.badea392)
Keywords: regression
Version: unspecified → Trunk
Comment on attachment 8933368 [details]
Bug 1408227 - part 2: WSRunObject::InsertBreak() should take |const EditorRawDOMPoint&| as an argument

https://reviewboard.mozilla.org/r/201916/#review209874


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: editor/libeditor/HTMLEditRules.cpp:1896
(Diff revision 1)
> +    EditorRawDOMPoint afterBRNode(brNode);
> +    DebugOnly<bool> advanced = afterBRNode.AdvanceOffset();
> +    NS_WARNING_ASSERTION(advanced,
> +      "Failed to advance offset after the new <br> node");
> +    node = afterBRNode.Container();
> +    aOffset = afterBRNode.Offset();

Warning: Value stored to 'aoffset' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]
Comment on attachment 8933367 [details]
Bug 1408227 - part 1: TextEditor::CreateBRImpl() should take |const EditorRawDOMPoint&| for insertion point of new <br> element

https://reviewboard.mozilla.org/r/201914/#review210316
Attachment #8933367 - Flags: review?(m_kato) → review+
Comment on attachment 8933368 [details]
Bug 1408227 - part 2: WSRunObject::InsertBreak() should take |const EditorRawDOMPoint&| as an argument

https://reviewboard.mozilla.org/r/201916/#review210318
Attachment #8933368 - Flags: review?(m_kato) → review+
Comment on attachment 8933369 [details]
Bug 1408227 - part 3: Redesign WSRunObject::FindRun() with EditorRawDOMPoint

https://reviewboard.mozilla.org/r/201918/#review210320
Attachment #8933369 - Flags: review?(m_kato) → review+
Comment on attachment 8933370 [details]
Bug 1408227 - part 4: WSRunObject::DeleteChars() should take two |const EditorRawDOMPoint&| arguments to specify a range to remove

https://reviewboard.mozilla.org/r/201920/#review210322
Attachment #8933370 - Flags: review?(m_kato) → review+
Comment on attachment 8933371 [details]
Bug 1408227 - part 5: Redesign GetCharAfter(), GetCharBefore(), GetWSPointAfter() and GetWSPointBefore() of WSRunObject

https://reviewboard.mozilla.org/r/201922/#review210326
Attachment #8933371 - Flags: review?(m_kato) → review+
Comment on attachment 8933372 [details]
Bug 1408227 - part 6: WSRunObject::CheckTrailingNBSP() should take |const EditorRawDOMPoint&| instead of a set of container node and offset in it

https://reviewboard.mozilla.org/r/201924/#review210328
Attachment #8933372 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/13d9a39c2f97
part 1: TextEditor::CreateBRImpl() should take |const EditorRawDOMPoint&| for insertion point of new <br> element r=m_kato
https://hg.mozilla.org/integration/autoland/rev/6fc7cf0d7a86
part 2: WSRunObject::InsertBreak() should take |const EditorRawDOMPoint&| as an argument r=m_kato
https://hg.mozilla.org/integration/autoland/rev/33d2e22dc648
part 3: Redesign WSRunObject::FindRun() with EditorRawDOMPoint r=m_kato
https://hg.mozilla.org/integration/autoland/rev/baed101e364b
part 4: WSRunObject::DeleteChars() should take two |const EditorRawDOMPoint&| arguments to specify a range to remove r=m_kato
https://hg.mozilla.org/integration/autoland/rev/ed55a1cbf4ab
part 5: Redesign GetCharAfter(), GetCharBefore(), GetWSPointAfter() and GetWSPointBefore() of WSRunObject r=m_kato
https://hg.mozilla.org/integration/autoland/rev/b4af5fae3d9b
part 6: WSRunObject::CheckTrailingNBSP() should take |const EditorRawDOMPoint&| instead of a set of container node and offset in it r=m_kato
Depends on: 1425997
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: