Closed Bug 1284125 Opened 8 years ago Closed 8 years ago

Intermittent devtools/client/inspector/test/browser_inspector_delete-selected-node-02.js | Have the correct number of breadcrumbs - Got 4, expected 3

Categories

(DevTools :: Inspector, defect, P2)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox50 fixed, firefox51 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: sjakthol)

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Bug 1284125 - Fix intermittent browser_inspector_delete-selected-node-02.js.
58 bytes, text/x-review-board-request
jdescottes
: review+
Details 
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P3
Inspector bug triage (filter on CLIMBING SHOES).
Priority: P3 → P2
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=66be669416d2
Assignee: nobody → sjakthol
Status: NEW → ASSIGNED
Comment on attachment 8780828 [details]
Bug 1284125 - Fix intermittent browser_inspector_delete-selected-node-02.js.

https://reviewboard.mozilla.org/r/71426/#review69064

Thanks for working on this and for the detailed investigation!
Only have 2 suggestions/comments, up to you if you want to consider them.

::: devtools/client/inspector/breadcrumbs.js:814
(Diff revision 1)
>                       cmdDispatcher.focusedElement.parentNode == this.container);
>  
>      if (!this.selection.isConnected()) {
>        // remove all the crumbs
>        this.cutAfter(-1);
>        return;

For consistency, should we also fire a "breadcrumbs-updated" event here?

::: devtools/client/inspector/test/browser_inspector_delete-selected-node-02.js:120
(Diff revision 1)
> +
> +    // Since the mutations are sent asynchronously from the server, the
> +    // inspector-updated event triggered by the deletion might happen before
> +    // the mutation is received and the element is removed from the
> +    // breadcrumbs. See bug 1284125.
> +    if (inspector.breadcrumbs.indexOf(nodeToBeDeleted) > -1) {

Maybe we can avoid the if statement here and simplify the logic. In all situations we expect to receive a breadcrumbs-updated and a inspector-updated event, we just don't know in which order.

Before synthesizing VK_ESCAPE, we could create listeners to both events, and yield only after :

>    let onInspectorUpdated = inspector.once("inspector-updated");
>    let onBreadcrumbsUpdated = inspector.once("breadcrumbs-updated");
>    EventUtils.synthesizeKey("VK_ESCAPE", {});
>    yield onInspectorUpdated;
>    yield onBreadcrumbsUpdated;

Would need to be checked on try though. Not mandatory since your implementation works fine here.
Attachment #8780828 - Flags: review?(jdescottes) → review+
Comment on attachment 8780828 [details]
Bug 1284125 - Fix intermittent browser_inspector_delete-selected-node-02.js.

https://reviewboard.mozilla.org/r/71426/#review69064

> For consistency, should we also fire a "breadcrumbs-updated" event here?

We could but it could also break something so I rather not do that :)

> Maybe we can avoid the if statement here and simplify the logic. In all situations we expect to receive a breadcrumbs-updated and a inspector-updated event, we just don't know in which order.
> 
> Before synthesizing VK_ESCAPE, we could create listeners to both events, and yield only after :
> 
> >    let onInspectorUpdated = inspector.once("inspector-updated");
> >    let onBreadcrumbsUpdated = inspector.once("breadcrumbs-updated");
> >    EventUtils.synthesizeKey("VK_ESCAPE", {});
> >    yield onInspectorUpdated;
> >    yield onBreadcrumbsUpdated;
> 
> Would need to be checked on try though. Not mandatory since your implementation works fine here.

I don't think that's going to work since whenever there's an inspector-updated event, there's also breadcrumbs-updated event emitted at [1]. So the test would still continue immediately after inspector-updated event is received and the problem is still there.

With this patch there will be exactly one inspector-updated event and either one or two breadcrumbs-updated events emitted depending if the deletion is observed before the initial inspector update finishes or if it's still pending when the mutations arrive from the server.

[1] https://dxr.mozilla.org/mozilla-central/rev/6e191a55c3d23e83e6a2e72e4e80c1dc21516493/devtools/client/inspector/breadcrumbs.js#872
I'm going to mark this as checkin-needed but I strongly encourage someone to take a really close look at the machinery inspector and related tools use to notify interested parties about updates. If there were clear rules in place on how tools should use the inspector-updated event, a lot of these intermittent test failures could be completely avoided.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/03274eafd91e
Fix intermittent browser_inspector_delete-selected-node-02.js. r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/03274eafd91e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
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: