Closed Bug 1345529 Opened 7 years ago Closed 7 years ago

[Regression] Partially elements may lost when remove element in Inspector

Categories

(DevTools :: Inspector, defect, P1)

Product:
DevTools
Component:
Inspector
Version:
52 Branch
Type:
defect

Tracking

(firefox-esr52 wontfix)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix

People

(Reporter: 684sigma, Assigned: jdescottes)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1345529 - fix inspector DocumentWaler children() method;
59 bytes, text/x-review-board-request
pbro
: review+
gchang
: approval-mozilla-beta-
Details 
I have a problem with Firefox Beta 52. It doesn't happen in Firefox ESR 45.
Sometimes when I edit HTML in inspector, it partially disappears.
It happens unpredictably, however, I noticed one specific scenario when it happens

1. Open this url (text between quotes is url): "data:text/html,<div><p><a>test1</a> <b>test2</b></p></div>"
2. Open inspector, delete node <b>

Result: all inner html of node <div> disappears in Inspector
Expected: shouldn't disappear
Blocks: 1304685
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → affected
Component: Untriaged → Developer Tools: Inspector
Ever confirmed: true
Summary: Regression HTML in Inspector partially disappears sometimes → [Regression] Partially elements may lost when remove element in Inspector
Keywords: regression
Flags: needinfo?(pbrosset)
Thank you for filing this issue. This has completely gone unnoticed in our tests.
I was able to reproduce the issue using the steps in comment 0.

The browser console also shows the following error when this happens:

DOMException [HierarchyRequestError: "Node cannot be inserted at the specified point in the hierarchy"
code: 3
nsresult: 0x80530003
location: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/markup/markup.js:1692]

Closing/re-opening the inspector fixes it.
I will triage/assign this bug later today.
Flags: needinfo?(pbrosset)
Priority: -- → P1
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Too late for a fix for 53, fix-optional for 54, minor carryover regression.
status-firefox53: affected → wontfix
status-firefox54: affected → fix-optional
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8c4a43a32b5265ffae38027feaaa4db72ad8f57

Note that there is still an issue after my fix.
When you apply the STRs from the logger, the selection remains on the whitespace text node that is no longer rendered in the markup view. Not sure yet how to deal with this so it would be nice if we could fix it in a follow up.
Comment on attachment 8855702 [details]
Bug 1345529 - fix inspector DocumentWaler children() method;

Forwarding this to pbro
Attachment #8855702 - Flags: review?(gl) → review?(pbrosset)
Comment on attachment 8855702 [details]
Bug 1345529 - fix inspector DocumentWaler children() method;

https://reviewboard.mozilla.org/r/127590/#review130486

This looks great. Thanks for digging into this code.

I have one general comment: I know this is only naming, but I think I'd prefer to use the words accepted/skipped instead of valid/invalid. When I hear that a node is invalid, I think about dead, or detached, or something, but not about just a normal node that happens to be skipped by the DocumentWalker filter. One example: I'd rename `getClosestValidNode` to `getClosestAcceptedNode`.

I have made a couple of minor comments below, but I also have a question about the logic change, and I'd like that cleared out before giving R+ because I'm not sure I follow.

::: commit-message-95061:3
(Diff revision 4)
> +Bug 1345529 - fix inspector walker children() method;r=gl
> +
> +The inspector actor walker had several issues when trying to retrieve

Maybe let's rephrase "inspector actor walker" to "DocumentWalker".

::: devtools/server/actors/inspector.js:2993
(Diff revision 4)
> +    if (filter(previous) === nodeFilterConstants.FILTER_ACCEPT) {
> +      // A valid node was found in the previous siblings of the startingNode.
> +      return previous;
> +    }
> +
> +    if (filter(next) === nodeFilterConstants.FILTER_ACCEPT) {
> +      // A valid node was found in the next siblings of the startingNode.
> +      return next;
> +    }

What is the effect (on the UI) of choosing the previous sibling by default when both a previous and next sibling exist and are valid?

::: devtools/server/actors/inspector.js:3047
(Diff revision 4)
> +    } else if (skipTo === nodeFilterConstants.SKIP_TO_SIBLING) {
> +      node = getClosestValidNode(node, filter);
> +    }

I'm not sure I understand how this part works.

Before your change, each iteration of the loop would move the walker to the parent (calling `this.walker.parentNode` would change `currentNode`).

Now, with your change, when we are in `SKIP_TO_SIBLING` mode, `currentNode` doesn't seem to be changed.
Let's say we do find an accepted sibling in `getClosestValidNode`, we exit the while loop, but the `currentNode` state of the walker hasn't changed.

Or am I reading this wrong?

Also, what happens when we can't find any accepted sibling?

::: devtools/shared/dom-node-filter-constants.js:22
(Diff revision 4)
> +  SKIP_TO_PARENT: "SKIP_TO_PARENT",
> +  SKIP_TO_SIBLING: "SKIP_TO_SIBLING"

Could you add a comment for these new constantes please?
I think all other constants here are standard things you can find documentation about on the internet, but these 2 new ones are custom, just for us.
In fact, because of this, maybe it's better to just define them as local constants inside the inspector.js module.
Attachment #8855702 - Flags: review?(pbrosset)
> What is the effect (on the UI) of choosing the previous sibling by default when
> both a previous and next sibling exist and are valid?

I guess it depends on the caller. Here we are only using this approach when fetching the
children of a container. So using the previous or next sibling really doesn't matter.

The only reason this looks complicated is because I am trying to make sure we get the 
closest sibling available, so I'm looping on both the previous and next siblings in each
step. 

This, again, is not really useful for our current caller, I probably could simply loop a 
first time on previous siblings and in case no accepted node was found, loop again on next 
siblings.

> I'm not sure I understand how this part works.
> 
> Before your change, each iteration of the loop would move the walker to the
> parent (calling `this.walker.parentNode` would change `currentNode`).
> 
> Now, with your change, when we are in `SKIP_TO_SIBLING` mode, `currentNode`
> doesn't seem to be changed.
> Let's say we do find an accepted sibling in `getClosestValidNode`, we exit the
> while loop, but the `currentNode` state of the walker hasn't changed.
> 
> Or am I reading this wrong?

Good catch, this is a leftover of another approach I tried before (I was selecting the sibling before creating the Walker, but since we already have a fallback mechanism in the walker, I decided to try and do everything there)

> Also, what happens when we can't find any accepted sibling?

We have a walker initialized on a non accepted node. We are still able to call all the 
methods from the document walker, but we should not trust the currentNode from the 
DocumentWalker as it might point to a filtered out node. But I don't think we should do
any other magic. Call sites should be careful about this because the DocumentWalker can 
already have a filtered-out currentNode (after calling next/previousSibling for instance)

> I think all other constants here are standard things you can find documentation
> about on the internet, but these 2 new ones are custom, just for us.
> In fact, because of this, maybe it's better to just define them as local
> constants inside the inspector.js module.

I agree, done.
Comment on attachment 8855702 [details]
Bug 1345529 - fix inspector DocumentWaler children() method;

https://reviewboard.mozilla.org/r/127590/#review130868

This looks good to me now. Thanks Julian.
Attachment #8855702 - Flags: review?(pbrosset) → review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fda9bea59c6f
fix inspector DocumentWaler children() method;r=pbro
Backed out in https://hg.mozilla.org/integration/autoland/rev/30fc8aa8a780 for timeouts in browser_markup_mutation_01.js (all platforms, e10s and not), e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=90112211&repo=autoland
walker.currentNode can't be null, which might now happen if the fallback mechanism could not find any non filtered node.
In this case I now use the node provided in the constructor to initialize the internal walker.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d508a91f09e9
fix inspector DocumentWaler children() method;r=pbro
https://hg.mozilla.org/mozilla-central/rev/d508a91f09e9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1355886
Depends on: 1355893
Going out on a limb that this isn't worth backporting to esr52. Is this worth consideration for Aurora backport or should it ride the trains?
status-firefox-esr52: fix-optional → wontfix
Flags: needinfo?(jdescottes)
This patch ideally needs to be uplifted together with Bug 1355886, which just made its way to autoland.
If we can get both uplifted let's do it. Otherwise I would prefer to skip it.

Given the current release schedule I guess it means we can hope to uplift it to 54beta?
Flags: needinfo?(jdescottes)
(In reply to Julian Descottes [:jdescottes] from comment #23)
> Given the current release schedule I guess it means we can hope to uplift it
> to 54beta?

That's correct. Please go ahead and do the requests if you think the patches are ready to go.
Flags: needinfo?(jdescottes)
Comment on attachment 8855702 [details]
Bug 1345529 - fix inspector DocumentWaler children() method;

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1304685
[User impact if declined]: markup view can be broken after deleting a node, forcing the user to restart devtools to get it back in a correct state
[Is this code covered by automated tests?]: yes, included in this patch
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: yes, see comment 0 
[List of other uplifts needed for the feature/fix]: the patch from Bug 1355886 should be uplifted too (will be requested separately)
[Is the change risky?]: no
[Why is the change risky/not risky?]: devtools only change, has been on central for some time.
[String changes made/needed]: none
Flags: needinfo?(jdescottes)
Attachment #8855702 - Flags: approval-mozilla-beta?
Hi Julian,
This issue has been there for a while and we don't have aurora phase to stabilize, can we let these 2 bugs ride the train and won't fix in 54?
Flags: needinfo?(jdescottes)
Hi Gerry,

I think this bug is embarrassing since it forces the user to restart devtools when it occurs. I was quite surprised we didn't get a report earlier than this, it might be because the STRs can be hard to identify.

I still think it's worth uplifting, but if you feel strongly against it feel free to cancel the requests.
Flags: needinfo?(jdescottes)
I have reproduced this bug with Nightly 55.0a1 (2017-03-08) on Windows 8.1 , 64 Bit ! 

This bug's fix is Verified with latest Nightly !

Build   ID    20170420030346
User Agent    Mozilla/5.0 (Windows NT 6.3; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170419]
Comment on attachment 8855702 [details]
Bug 1345529 - fix inspector DocumentWaler children() method;

Thanks for understanding. Let's let it ride the train on 55. Beta54-.
Attachment #8855702 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: