Closed Bug 1366853 Opened 7 years ago Closed 7 years ago

source-map-url-service should wait for sources

Categories

(DevTools :: Framework, enhancement, P3)

enhancement

Tracking

(firefox55 fixed, firefox56 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

If you open the console after the page calls console.log, then the log messages won't have source maps applied. This happens because source-map-url-service doesn't request the current sources from the thread actor. Simple example here: https://tromey.github.io/source-map-examples/simple/index.html
This is similar to bug 1364526, but in that case the problem is that nothing in the new debugger's view-source code path waits for sources to be loaded.
Comment on attachment 8870181 [details] Bug 1366853 - SourceMapURLService must wait for sources to be available; https://reviewboard.mozilla.org/r/141626/#review145276 Went ahead and did a try push for talos since I believe this will run eagerly at startup (at least in the console case): https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=679b9f080a5e767a630c2eab1d274e720456242f&newProject=try&newRevision=303aa52bd00e71526e3dc280d97137befc3eb26d&framework=1&filter=damp&showOnlyImportant=0 ::: devtools/client/framework/test/browser_source_map-no-race.js:9 (Diff revision 1) > +// Test that the source map service doesn't race against source > +// reporting. > + > +"use strict"; > + > +const PAGE_URL = URL_ROOT + "doc_no_race.html"; I prefer to use a data uri if there's no need for this to be an external file (I don't think there is in this case, but feel free to disregard if there is) ::: devtools/client/framework/test/browser_source_map-no-race.js:28 (Diff revision 1) > + info(`checking original location for ${JS_URL}:${GENERATED_LINE}`); > + let newLoc = yield service.originalPositionFor(JS_URL, GENERATED_LINE); > + is(newLoc.sourceUrl, ORIGINAL_URL, "check mapped URL"); > + is(newLoc.line, ORIGINAL_LINE, "check mapped line number"); > + > + yield toolbox.destroy(); I don't think you need these 3 lines, the shared-head should clean up for you in a registerCleanupFunction
Attachment #8870181 - Flags: review?(bgrinstead) → review+
This broke some other tests and in the end I think maybe it is related to the debugger startup race. In particular: https://github.com/devtools-html/debugger.html/blob/master/src/client/firefox.js#L32-L33 I think this code needs to await for the result of newSources
(In reply to Brian Grinstead [:bgrins] from comment #4) > Correction on the talos URL, this should be the right one: > https://treeherder.mozilla.org/perf.html#/ > compare?originalProject=try&originalRevision=2a3685681a2e7be1ca787087e12712cb > fad7957e&newProject=try&newRevision=ac376e67f24fdfdd71cf271a5a4d23886e0e3b22& > framework=1&filter=damp&showOnlyImportant=0 Talos looks fine
This is going to need the patch for bug 1364526
Depends on: 1364526
DevTools bug triage (filter on CLIMBING SHOES).
Priority: -- → P3
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try searching for changes remote: waiting for lock on working directory of /repo/hg/mozilla/try held by process '30997' on host 'hgssh4.dmz.scl3.mozilla.com' remote: abort: working directory of /repo/hg/mozilla/try: timed out waiting for lock held by hgssh4.dmz.scl3.mozilla.com:30997 abort: stream ended unexpectedly (got 0 bytes, expected 4)
I think this needs bug 1368695 now.
Depends on: 1368695
I misunderstood how getSources is to be called, so was getting a failure. I think this should be a bit better.
Pushed by ttromey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/30c2b38c4a0f SourceMapURLService must wait for sources to be available; r=bgrins
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Backed out for frequently failing browser_source_map-no-race.js at line 39 with: newLoc is null: https://hg.mozilla.org/integration/autoland/rev/b609d94a575a85e6228f751893d6324683510670 Push with first failure (other ones are on Linux x64 pgo): https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=39b358a1d0ca52dd1a0292700e424853131b366f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=104085560&repo=autoland 08:08:32 INFO - GECKO(2164) | ************************* 08:08:32 INFO - GECKO(2164) | A coding exception was thrown and uncaught in a Task. 08:08:32 INFO - GECKO(2164) | Full message: TypeError: newLoc is null 08:08:32 INFO - GECKO(2164) | Full stack: @chrome://mochitests/content/browser/devtools/client/framework/test/browser_source_map-no-race.js:39:3 08:08:32 INFO - GECKO(2164) | Tester_execTest@chrome://mochikit/content/browser-test.js:774:9 08:08:32 INFO - GECKO(2164) | Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:686:7 08:08:32 INFO - GECKO(2164) | SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59 08:08:32 INFO - GECKO(2164) | ************************* 08:08:32 INFO - TEST-INFO | started process screenshot 08:08:32 INFO - TEST-INFO | screenshot: exit 0 08:08:32 INFO - Buffered messages logged at 08:08:28 08:08:32 INFO - Entering test bound 08:08:32 INFO - Adding a new tab with URL: data:text/html, 08:08:32 INFO - <!doctype html> 08:08:32 INFO - 08:08:32 INFO - <html> 08:08:32 INFO - <script src="http://example.com/browser/devtools/client/framework/test/code_bundle_no_race.js"></script> 08:08:32 INFO - <head> 08:08:32 INFO - <meta charset="utf-8"/> 08:08:32 INFO - <title>Empty test page to test race case</title> 08:08:32 INFO - </head> 08:08:32 INFO - 08:08:32 INFO - <body> 08:08:32 INFO - </body> 08:08:32 INFO - 08:08:32 INFO - </html> 08:08:32 INFO - Buffered messages logged at 08:08:29 08:08:32 INFO - Tab added and finished loading 08:08:32 INFO - Opening the toolbox 08:08:32 INFO - Buffered messages logged at 08:08:32 08:08:32 INFO - Toolbox opened and focused 08:08:32 INFO - checking original location for http://example.com/browser/devtools/client/framework/test/code_bundle_no_race.js:84 08:08:32 INFO - Buffered messages finished 08:08:32 INFO - TEST-UNEXPECTED-FAIL | devtools/client/framework/test/browser_source_map-no-race.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/framework/test/browser_source_map-no-race.js:39 - TypeError: newLoc is null 08:08:32 INFO - Stack trace: 08:08:32 INFO - @chrome://mochitests/content/browser/devtools/client/framework/test/browser_source_map-no-race.js:39:3 08:08:32 INFO - Tester_execTest@chrome://mochikit/content/browser-test.js:774:9 08:08:32 INFO - Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:686:7 08:08:32 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59 08:08:32 INFO - Tester_execTest@chrome://mochikit/content/browser-test.js:774:9 08:08:32 INFO - Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:686:7 08:08:32 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59 08:08:32 INFO - Leaving test bound
Status: RESOLVED → REOPENED
Flags: needinfo?(ttromey)
Resolution: FIXED → ---
My current hypothesis is that the script that does the logging is being GC'd before the toolbox opens. Some logging seems to support this, in that the toolbox reports no sources at startup -- but the toolbox shouldn't be opened until the page is completely loaded.
Flags: needinfo?(ttromey)
Another theory destroyed, to be found in the midden by future archaeologists. https://treeherder.mozilla.org/logviewer.html#?job_id=104976127&repo=try&lineNumber=3689 No new theory in sight yet.
Thanks to :jdescottes. It seems that the problem was a misplaced <script> tag in the test case. https://treeherder.mozilla.org/#/jobs?repo=try&revision=6541e7ce295ff54cc952bb84dcb36465f97bb0be ... which doesn't look super, but those reds are some other intermittent.
...doing a few extra spins of dt4 just to be sure.
Pushed by ttromey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cff7d7889369 SourceMapURLService must wait for sources to be available; r=bgrins
Backed out in https://hg.mozilla.org/integration/autoland/rev/aa3ad194df6c for the same again, browser_source_map-no-race.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/framework/test/browser_source_map-no-race.js:39 - TypeError: newLoc is null, https://treeherder.mozilla.org/logviewer.html#?job_id=105693357&repo=autoland and all the ones I starred as bug 1369912 before I realized why I was starring massive failure as a closed bug.
I'm sorry about that.
This update is based on the theory that the script was being GC'd and so wasn't seen during load. Here's a try run with some retriggers of the job that should fail: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a586576789795f515110d7fc85470673823b355c After the last round though it is hard to be confident in this kind of thing :(
The reason this patch seems better than what I rejected in comment #20 is that I'm not totally sure that the previous method of trying to prevent GC would actually work -- but setting something on |window| definitely will.
Pushed by ttromey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ddc3eb7701a3 SourceMapURLService must wait for sources to be available; r=bgrins
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: