Closed Bug 1342051 Opened 3 years ago Closed 3 years ago

Grid highlighter can no longer be activated after Inspector close and page refresh

Categories

(DevTools :: Inspector, defect, P3, major)

defect

Tracking

(firefox51 unaffected, firefox52+ verified, firefox-esr52 verified, firefox53 verified, firefox54 verified)

VERIFIED FIXED
Firefox 54
Tracking Status
firefox51 --- unaffected
firefox52 + verified
firefox-esr52 --- verified
firefox53 --- verified
firefox54 --- verified

People

(Reporter: pauly, Assigned: jdescottes)

References

(Blocks 3 open bugs)

Details

Attachments

(4 files, 1 obsolete file)

[Affected versions]:
- 52b8, 54.0a1 (2017-02-23)

[Affected platforms]:
- Windows 7 x86, macOS 10.11, Ubuntu 14.04 x64

[Steps to reproduce]:
1. Start Firefox
2. Open http://labs.jensimmons.com/examples/grid-content-1.html
3. Open inspector, click on <main> and activate the grid tool
4. Close the inspector
5. Reload page
6. Open inspector, click on <main> and try to activate the grid tool again

[Expected result]:
- Grid highlighter is properly displayed.

[Actual result]:
- The grid does not work. Browser console log:
<unavailable>  protocol.js:940
Empty histogram keys are not allowed. (unknown)
A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Date: Thu Feb 23 2017 15:56:00 GMT+0200 (GTB Standard Time)
Full Message: Protocol error (unknownError): can't access dead object
Full Stack: JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: register :: line 199
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: completePromise :: line 711
JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js :: _handleException :: line 442
JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js :: _run :: line 323
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: process :: line 925
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: walkerLoop :: line 806

[Regression range]:
- not a regression, repro on Nightly 2016-11-20
Severity: normal → major
Priority: -- → P3
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
It looks like the cache used to store "gap" patterns for the grid highlighter is persisted when following the STRs.
Cleaning the cache during the highlighter's destroy() fixes the issue and should be easy to uplift.
Comment on attachment 8841639 [details]
Bug 1342051 - clear css grid pattern cache when destroying highlighter;

https://reviewboard.mozilla.org/r/115800/#review117178

Looks good to me, but I would love to have more unit test in grid inspector: this one could mimic the STR mentioned in the bug (so it will fails due the timeout currently and it will succeed with the patch applied, I guess).
If you see that the test it will be harder to write than expected, just file a new bug about it and we can land this one in the meantime.
Attachment #8841639 - Flags: review?(zer0) → review+
Flags: qe-verify+
Thanks for the review. I thought we had no tests for the grid highlighter so I didn't look into this, sorry!

Added a test as a separate changeset. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a42f3a130c073321733b3d0781c814de7c8f3ca9
Comment on attachment 8841699 [details]
Bug 1342051 - add test for grid highlighter bug after reload;

https://reviewboard.mozilla.org/r/115844/#review117362

Looks good to me Julian! r+ with the comment addressed.

::: devtools/client/inspector/rules/test/browser_rules_grid-highlighter-on-reload.js:33
(Diff revision 1)
> +
> +  info("Close the toolbox before reloading the tab");
> +  let target = TargetFactory.forTab(gBrowser.selectedTab);
> +  yield gDevTools.closeToolbox(target);
> +
> +  refreshTab(gBrowser.selectedTab);

You probably need to `yield` here if we want to be sure the tab has finished before check the grid inspector again.
Comment on attachment 8841699 [details]
Bug 1342051 - add test for grid highlighter bug after reload;

https://reviewboard.mozilla.org/r/115844/#review117366
Attachment #8841699 - Flags: review?(zer0) → review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a3859b017684
add test for grid highlighter bug after reload;r=zer0
https://hg.mozilla.org/integration/autoland/rev/f32a30e62641
clear css grid pattern cache when destroying highlighter;r=zer0
https://hg.mozilla.org/mozilla-central/rev/a3859b017684
https://hg.mozilla.org/mozilla-central/rev/f32a30e62641
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Please request uplift to aurora and beta ASAP so we can get this in the 52 release.
Flags: needinfo?(jdescottes)
[bugday-20170301]
Attached patch patch for aurora (obsolete) — Splinter Review
Folded both changesets into one and rebased on top of aurora

Approval Request Comment
[Feature/Bug causing the regression]:bug in new feature
[User impact if declined]: CSS grid highlighter no longer works after reloading the page
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: yes
1. Start Firefox
2. Open http://labs.jensimmons.com/examples/grid-content-1.html
3. Open inspector, click on <main> and activate the grid tool
4. Close the inspector
5. Reload page
6. Open inspector, click on <main> and try to activate the grid tool again

[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: devtools only and simple JS fix to clear a cache object.
[String changes made/needed]: none
Flags: needinfo?(jdescottes)
Attachment #8842773 - Flags: review+
Attachment #8842773 - Flags: approval-mozilla-aurora?
Attachment #8842773 - Flags: approval-mozilla-aurora?
Comment on attachment 8842773 [details] [diff] [review]
patch for aurora

needs more work
Attachment #8842773 - Attachment is obsolete: true
Folded both changesets into one and rebased on top of aurora + fixed a small bug with the initial rebased version.
 
Approval Request Comment
[Feature/Bug causing the regression]:bug in new feature
[User impact if declined]: CSS grid highlighter no longer works after reloading the page
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: yes
1. Start Firefox
2. Open http://labs.jensimmons.com/examples/grid-content-1.html
3. Open inspector, click on <main> and activate the grid tool
4. Close the inspector
5. Reload page
6. Open inspector, click on <main> and try to activate the grid tool again

[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: devtools only and simple JS fix to clear a cache object.
[String changes made/needed]: none
Attachment #8842777 - Flags: review+
Attachment #8842777 - Flags: approval-mozilla-aurora?
Attached patch patch for betaSplinter Review
Patch has been rebased on beta and tested manually. The automated test has also been fixed for beta. 
Try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fe559db50a9431c56584596cc067ce9ed2e3145

Approval Request Comment
[Feature/Bug causing the regression]:bug in new feature
[User impact if declined]: CSS grid highlighter no longer works after reloading the page
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: yes
1. Start Firefox
2. Open http://labs.jensimmons.com/examples/grid-content-1.html
3. Open inspector, click on <main> and activate the grid tool
4. Close the inspector
5. Reload page
6. Open inspector, click on <main> and try to activate the grid tool again

[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: devtools only and simple JS fix to clear a cache object.
[String changes made/needed]: none
Attachment #8842798 - Flags: review+
Attachment #8842798 - Flags: approval-mozilla-beta?
Attachment #8842777 - Attachment is patch: true
Comment on attachment 8842798 [details] [diff] [review]
patch for beta

fix an annoying issue with css grid inspector for 52

Last minute fix for 52 rc2.
Attachment #8842798 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8842777 [details] [diff] [review]
patch for aurora - v2

css grid inspector fix for aurora53.
Attachment #8842777 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed FX 52.0-build2 on Win 7 x64, Win 10 x64, OS X 10.11, Ubuntu 16.04 x86.
Verified fixed FX 52 ESR, 53.0a2 (2017-03-06), 54.0a1 (2017-03-06) Win 10
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.