Closed
Bug 1366853
Opened 7 years ago
Closed 7 years ago
source-map-url-service should wait for sources
Categories
(DevTools :: Framework, enhancement, P3)
DevTools
Framework
Tracking
(firefox55 fixed, firefox56 fixed)
RESOLVED
FIXED
Firefox 55
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
Assignee | ||
Comment 1•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
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+
Comment 4•7 years ago
|
||
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
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
Comment 7•7 years ago
|
||
(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
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
I misunderstood how getSources is to be called, so was getting a failure.
I think this should be a bit better.
Comment 16•7 years ago
|
||
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/30c2b38c4a0f
SourceMapURLService must wait for sources to be available; r=bgrins
Comment 17•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment 18•7 years ago
|
||
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 → ---
Assignee | ||
Comment 19•7 years ago
|
||
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)
Assignee | ||
Comment 20•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•7 years ago
|
||
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.
Assignee | ||
Comment 23•7 years ago
|
||
...doing a few extra spins of dt4 just to be sure.
Comment 24•7 years ago
|
||
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cff7d7889369
SourceMapURLService must wait for sources to be available; r=bgrins
Comment 25•7 years ago
|
||
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.
Assignee | ||
Comment 26•7 years ago
|
||
I'm sorry about that.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 28•7 years ago
|
||
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 :(
Assignee | ||
Comment 29•7 years ago
|
||
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.
Comment 30•7 years ago
|
||
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ddc3eb7701a3
SourceMapURLService must wait for sources to be available; r=bgrins
Comment 31•7 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•