Closed Bug 1599635 Opened 1 year ago Closed 1 year ago

Inspector markup view stays empty when refreshing jsfiddle

Categories

(DevTools :: Inspector, defect, P1)

defect

Tracking

(firefox-esr68 unaffected, firefox70 unaffected, firefox71 unaffected, firefox72 fixed, firefox73 fixed)

RESOLVED FIXED
Firefox 73
Tracking Status
firefox-esr68 --- unaffected
firefox70 --- unaffected
firefox71 --- unaffected
firefox72 --- fixed
firefox73 --- fixed

People

(Reporter: Harald, Assigned: mtigley)

References

(Regression)

Details

(Keywords: regression)

Attachments

(3 files)

Attached file browser-console.log

Was editing JSFiddle and was focused on the output iframe. On refresh, Inspector markup view was empty.

Attached image image.png
Assignee: nobody → mtigley
Status: NEW → ASSIGNED
Priority: -- → P1

There's a timing issue with the querySelector request to the server at https://searchfox.org/mozilla-central/source/devtools/shared/fronts/walker.js#140 . I've observed the following:

  • response returns an object only containing the "from" property. We are also expecting "node" and "newParents" on this object.

  • If we put a time delay before calling super.querySelector, then we get the expected object back and the markup view shows as expected.

I'm unfamiliar with this and need some guidance on knowing where to narrow down this timing issue. Julian, do you think you can give some tips on where to start looking?

Flags: needinfo?(jdescottes)

was focused on the output iframe

For the record, if anyone has trouble reproducing this bug, the previous sentence does not mean that you have to select the iframe in the frame selector. Only that you should select an element that is in the output iframe.

The STRs are:

  • open https://jsfiddle.net/6Lq5whk1/1/
  • open DevTools > Inspector
  • inspect the "INSPECTME" div
  • reload the page
    (note that this is "slightly" intermittent, as in "it works once in a while". but normally it should fail more often than not :) )

I'm unfamiliar with this and need some guidance on knowing where to narrow down this timing issue.
Julian, do you think you can give some tips on where to start looking?

Your analysis is correct, this is a timing issue!

Initially when reading the symptoms you described, I was wondering if this could be a case of protocol.js mixing two responses (our code is a bit fragile around this). But I think it's more straightforward than this, and it highlights a flaw in the logic added in Bug 1590050 to reselect nodes in iframes after reloading the page.

As we can see in the stack trace, the call to querySelector comes from the async findNodeFront(nodeSelectors) method (also in the walker front). The idea in findNodeFront is that if we have several selectors, it means the selection is nested in an iframe. And therefore, in order to get the "final" node front, for each selector but the last we do:

  • get the iframe node front
  • wait for iframe to be loaded
  • go to the next selector

(https://searchfox.org/mozilla-central/rev/f66eb70d175a2017a74b4f61ed556d5835c8a9db/devtools/shared/fronts/walker.js#514-537)

But what if the iframe is missing when we reload? There's nothing in the code to handle this, we assume the selector will return a valid node front. JSFiddle most likely creates the output iframe after the page was loaded, and we try to reselect the node before the iframe is created. So we can't find the iframe node front, and the code crashes. I think the only thing to do here is to bail out if we can't find a node front. We can't possibly wait for all iframes to be dynamically created after a reload (some might even never be recreated!)

(we should definitely uplift the fix if we miss the 72 release window)

response returns an object only containing the "from" property. We are also expecting "node" and "newParents" on this object.

That's a very good point, we should look at this in more details and hopefully improve the situation.

The fact that the response only contains "from" is because the walker actor's querySelector returns null when the node can't be found (https://searchfox.org/mozilla-central/rev/f66eb70d175a2017a74b4f61ed556d5835c8a9db/devtools/server/actors/inspector/walker.js#1220).

This is indeed confusing because it can lead to think that the response is "wrong". I am not 100% sure why protocol.js allows this, because if you look at the walker specs, the return value of querySelector is disconnectedNode:

    querySelector: {
      request: {
        node: Arg(0, "domnode"),
        selector: Arg(1),
      },
      response: RetVal("disconnectedNode"),
    },

Normally, nullable types are marked as nullable, so I would have expected to see nullable:disconnectedNode here. But maybe protocol.js allows all responses to take a null value. Anyway we could change that by returning { node: null, newParents: [] } in querySelector, in the actor implementation. That won't change the crash here, but that would make the investigation less confusing (note that you will have to update the specs of disconnectedNode to allow node to take null values, otherwise protocol.js will complain).

I would suggest to fix this in a follow up bug.

Flags: needinfo?(jdescottes)
Regressed by: 1590050

(In reply to Julian Descottes [:jdescottes] from comment #4)

As we can see in the stack trace, the call to querySelector comes from the async findNodeFront(nodeSelectors) method (also in the walker front). The idea in findNodeFront is that if we have several selectors, it means the selection is nested in an iframe. And therefore, in order to get the "final" node front, for each selector but the last we do:

  • get the iframe node front
  • wait for iframe to be loaded
  • go to the next selector

(https://searchfox.org/mozilla-central/rev/f66eb70d175a2017a74b4f61ed556d5835c8a9db/devtools/shared/fronts/walker.js#514-537)

But what if the iframe is missing when we reload? There's nothing in the code to handle this, we assume the selector will return a valid node front. JSFiddle most likely creates the output iframe after the page was loaded, and we try to reselect the node before the iframe is created. So we can't find the iframe node front, and the code crashes. I think the only thing to do here is to bail out if we can't find a node front. We can't possibly wait for all iframes to be dynamically created after a reload (some might even never be recreated!)

Thank you Julian for the thorough explanation of how this part of the code works. I really appreciate it. I think bailing out when the node front is missing is the best way to go and I think it's safe to assume returning just the root node will suffice?

The fact that the response only contains "from" is because the walker actor's querySelector returns null when the node can't be found (https://searchfox.org/mozilla-central/rev/f66eb70d175a2017a74b4f61ed556d5835c8a9db/devtools/server/actors/inspector/walker.js#1220).

This is indeed confusing because it can lead to think that the response is "wrong". I am not 100% sure why protocol.js allows this, because if you look at the walker specs, the return value of querySelector is disconnectedNode:

    querySelector: {
      request: {
        node: Arg(0, "domnode"),
        selector: Arg(1),
      },
      response: RetVal("disconnectedNode"),
    },

Normally, nullable types are marked as nullable, so I would have expected to see nullable:disconnectedNode here. But maybe protocol.js allows all responses to take a null value. Anyway we could change that by returning { node: null, newParents: [] } in querySelector, in the actor implementation. That won't change the crash here, but that would make the investigation less confusing (note that you will have to update the specs of disconnectedNode to allow node to take null values, otherwise protocol.js will complain).

I would suggest to fix this in a follow up bug.

Sounds good. I've created the appropriate issue for this: Bug 1601003

Since we did miss the window to land this in 72, let's make sure we uplift the fix whenever it's ready. Thanks for working on this Micah and Julian!

Pushed by mtigley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a3d8a3308f85
Return the root node if no valid iframe nodeFront can be found after a reload. r=jdescottes
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 73

Comment on attachment 9113051 [details]
Bug 1599635 - Return the root node if no valid iframe nodeFront can be found after a reload.

Beta/Release Uplift Approval Request

  • User impact if declined: Users who inspect content on the output frame of JSFiddle and reload the page will see an empty markup view. In general, this extends to reloading any page that dynamically creates and adds iframes.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It's a small change that only affects the DevTools Inspector
  • String changes made/needed:
Attachment #9113051 - Flags: approval-mozilla-beta?

Comment on attachment 9113051 [details]
Bug 1599635 - Return the root node if no valid iframe nodeFront can be found after a reload.

devtools regression fix, approved for 72.0b3

Attachment #9113051 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.