Closed Bug 1441462 Opened 2 years ago Closed 2 years ago

Layout sidebar panel crash on stripe.com/connect

Categories

(DevTools :: Inspector, defect, P1)

defect

Tracking

(firefox-esr52 unaffected, firefox60+ fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox-esr52 --- unaffected
firefox60 + fixed

People

(Reporter: pbro, Assigned: pbro)

Details

(Keywords: regression)

Attachments

(2 files)

Attached image sidebar.PNG
Here's what I did:

- opened stripe.com/connect
- opened the inspector with the layout tab visible
- clicked on each and every checkbox in the list of grids, one by one

At some stage, not sure which grid, the whole sidebar panel crashed and became empty.
The rest of the tools still worked, and the page was fine too.

See the screenshot.

Also, here's what was in the browser console when that happened:

TypeError: gridFragments[0] is undefined
Stack trace:
getTotalWidthAndHeight@resource://devtools/shared/base-loader.js -> resource://devtools/client/inspector/grids/components/GridOutline.js:177:11
componentWillReceiveProps@resource://devtools/shared/base-loader.js -> resource://devtools/client/inspector/grids/components/GridOutline.js:78:31
callComponentWillReceiveProps@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:6244:5
updateClassInstance@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:6419:7
updateClassComponent@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:7537:22
beginWork@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:7883:16
performUnitOfWork@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:9706:16
workLoop@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:9753:26
renderRoot@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:9832:9
performWorkOnRoot@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:10465:24
performWork@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:10418:7
batchedUpdates@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:10537:9
batchedUpdates@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:2203:12
dispatchEvent@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:3353:5
EventListener.handleEvent*listen@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:3163:7
trapBubbledEvent@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:3313:10
listenTo@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:3676:9
ensureListeningTo@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:11899:3
setInitialDOMProperties@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:11980:9
setInitialProperties$1@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:12130:3
finalizeInitialChildren@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:12637:5
completeWork@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:8306:19
completeUnitOfWork@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:9631:18
performUnitOfWork@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:9709:14
workLoop@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:9753:26
renderRoot@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:9832:9
performWorkOnRoot@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:10465:24
performWork@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:10418:7
requestWork@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:10329:7
scheduleWorkImpl@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:10188:11
scheduleWork@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:10152:12
enqueueSetState@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js:6131:7
Component.prototype.setState@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react.js:346:3
addTab@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/components/tabs/TabBar.js:113:5
addTab@resource://devtools/shared/base-loader.js -> resource://devtools/client/inspector/toolsidebar.js:97:5
addExistingTab@resource://devtools/shared/base-loader.js -> resource://devtools/client/inspector/toolsidebar.js:118:5
addRuleView@chrome://devtools/content/inspector/inspector.js:645:7
async*setupSidebar@chrome://devtools/content/inspector/inspector.js:709:11
async*_deferredOpen@chrome://devtools/content/inspector/inspector.js:277:5
async*Inspector.prototype.init<@chrome://devtools/content/inspector/inspector.js:163:18
_run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:310:39
process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:926:23
walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:810:7
Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:743:11
schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:774:7
completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:711:7
onPacket/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1339:9
DevTools RDP*request@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1279:14
generateRequestMethods/</frontProto[name]@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1433:14
WalkerFront<.querySelector<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/fronts/inspector.js:158:12
_getDefaultNodeForSelection/<@chrome://devtools/content/inspector/inspector.js:345:14
process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:926:23
walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:810:7
Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:743:11
schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:774:7
completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:711:7
onPacket/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1339:9
DevTools RDP*request@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1279:14
generateRequestMethods/</frontProto[name]@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1433:14
InspectorFront<.getPageStyle<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/fronts/inspector.js:483:12
_getPageStyle@chrome://devtools/content/inspector/inspector.js:305:12
Inspector.prototype.init<@chrome://devtools/content/inspector/inspector.js:156:11
_run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:310:39
promise callback*_handleResultValue@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:386:7
_run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:318:13
process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:926:23
walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:810:7
Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:743:11
schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:774:7
completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:711:7
onPacket/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1339:9
DevTools RDP*request@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1279:14
generateRequestMethods/</frontProto[name]@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1433:14
initCssProperties<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/fronts/css-properties.js:242:16
_run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:272:3
asyncFunction@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:246:14
Inspector.prototype.init<@chrome://devtools/content/inspector/inspector.js:153:33
_run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:272:3
Tracking 60 since we really should fix this in time for 60.
I could not reproduce on 59.
Is anyone looking at this?  Do we have a regression range?
So, we have 2 options here:
- either hide the grid from the list if it has no fragments
- or show it but handle the lack of fragments correctly

On the example I gave in comment 0, the lack of fragments comes from the fact that some grid elements are display:none when the page loads.

With option 1, in updateGridPanel (in grid-inspector.js), it would be easy enough to skip over gridFronts that don't have fragments.
Anyway, whenever there's a reflow, we refresh the panel, so as soon as display:none goes away, the grids would be displayed in the panel.

With option 2, the fix is just as easy: everywhere we access gridFragments[0], we should first check that the array isn't empty.
This way, the hidden grids still appear in the list, but clicking on them doesn't do anything visible (no outline appears, and no highlighter either).

Gabriel: what do you think is the right solution to choose here?
Flags: needinfo?(gl)
I'll push a commit for option 2. I think it's probably the best solution.
Comment on attachment 8956596 [details]
Bug 1441462 - Avoid crashing the gridOutline when there are no fragments;

https://reviewboard.mozilla.org/r/225518/#review231404
Attachment #8956596 - Flags: review?(gl) → review+
Flags: needinfo?(gl)
Assignee: nobody → pbrosset
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8aaf118e2e16
Avoid crashing the gridOutline when there are no fragments; r=gl
https://hg.mozilla.org/mozilla-central/rev/8aaf118e2e16
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.