Closed
Bug 1408227
Opened 8 years ago
Closed 8 years ago
Correctly update the child at offset pointer across calls to WSRunObject::InsertBreak()
Categories
(Core :: DOM: Editor, defect, P1)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla59
| Tracking | Status | |
|---|---|---|
| firefox57 | --- | unaffected |
| firefox58 | --- | wontfix |
| firefox59 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: masayuki)
References
Details
Attachments
(6 files, 1 obsolete file)
|
59 bytes,
text/x-review-board-request
|
m_kato
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
m_kato
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
m_kato
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
m_kato
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
m_kato
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
m_kato
:
review+
|
Details |
No description provided.
| Reporter | ||
Comment 1•8 years ago
|
||
Attachment #8918064 -
Flags: review?(masayuki)
| Reporter | ||
Comment 2•8 years ago
|
||
This patch fixes the problem I talked about in bug 1407966 comment 9.
| Reporter | ||
Updated•8 years ago
|
Keywords: regression
| Assignee | ||
Comment 3•8 years ago
|
||
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
Comment 5•8 years ago
|
||
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: ??? (???:???)
| Reporter | ||
Comment 6•8 years ago
|
||
This bug needs an owner to debug the test failure and reland this...
Comment 7•8 years ago
|
||
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.
Updated•8 years ago
|
Assignee: ehsan → catalin.badea392
Updated•8 years ago
|
Priority: -- → P1
Comment 8•8 years ago
|
||
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)
Updated•8 years ago
|
status-firefox57:
--- → unaffected
status-firefox58:
--- → affected
| Assignee | ||
Comment 9•8 years ago
|
||
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
| Assignee | ||
Updated•8 years ago
|
Attachment #8918064 -
Attachment is obsolete: true
| Assignee | ||
Comment 10•8 years ago
|
||
| Assignee | ||
Comment 11•8 years ago
|
||
| Assignee | ||
Comment 12•8 years ago
|
||
| Assignee | ||
Comment 13•8 years ago
|
||
| Assignee | ||
Comment 14•8 years ago
|
||
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
Comment 21•8 years ago
|
||
| mozreview-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/#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]
| Assignee | ||
Comment 22•8 years ago
|
||
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
Comment 28•8 years ago
|
||
| mozreview-review | ||
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 29•8 years ago
|
||
| mozreview-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 30•8 years ago
|
||
| mozreview-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 31•8 years ago
|
||
| mozreview-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 32•8 years ago
|
||
| mozreview-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 33•8 years ago
|
||
| mozreview-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+
Comment 34•8 years ago
|
||
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
Comment 35•8 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/13d9a39c2f97
https://hg.mozilla.org/mozilla-central/rev/6fc7cf0d7a86
https://hg.mozilla.org/mozilla-central/rev/33d2e22dc648
https://hg.mozilla.org/mozilla-central/rev/baed101e364b
https://hg.mozilla.org/mozilla-central/rev/ed55a1cbf4ab
https://hg.mozilla.org/mozilla-central/rev/b4af5fae3d9b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•