New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 687772 link

Starred by 2 users

Issue metadata

Status: Archived
Owner:
Last visit 29 days ago
Closed: Oct 31
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug



Sign in to add a comment

[devtools] kill source maps bugs with fire

Reported by antonin....@gmail.com, Feb 2 2017

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.76 Safari/537.36

Steps to reproduce the problem:
More complex source mapping might confuse DevTools (as reported in https://github.com/binaryage/dirac/issues/53)

We use ClojureScript and source maps it generates exposed some buggy edge cases in DevTools implementation. I believe those issues are not just ClojureScript-specific and might apply to other compile-to-js languages as well.

One class of problems are bugs in parsing source maps:
1. https://github.com/binaryage/dirac/commit/2f1268bd32b848d88ad6521ebf9f4e044cb2a061
2. https://github.com/binaryage/dirac/commit/adba5772a7f9969ba70f3707c89afc624a52da24

Second problem is the assumption that names are unique. Unfortunately Sources.SourceMapNamesResolver uses names as keys for mapping which fails in the face of non-unique names.
https://github.com/binaryage/dirac/commit/65c926461a373da92887ee37b35ed4f152b47297

Third problem was in source code decoration code. Again names were used to maintain mapping between properties and widgets, which does not work reliably with duplicate names. AFAIK, the code was written before source mapping support was added.
https://github.com/binaryage/dirac/commit/0ed1b3baff72be9b48a99a0a2b99ef30994bbe3b

What is the expected behavior?
source maps support to be rock-solid :-)

What went wrong?
Please follow https://github.com/binaryage/dirac/issues/53 for deeper discussion.

Did this work before? No 

Chrome version: 58.0.2999.0  Channel: canary
OS Version: OS X 10.12.4
Flash Version: Shockwave Flash 24.0 r0

Feel free to grab the patches from github if you find the code applicable.

Also please note that Sources.SourceMapNamesResolver.RemoteObject will probably need an updated API. See https://github.com/binaryage/dirac/issues/53#issuecomment-276740840

 
Owner: lushnikov@chromium.org
Status: Assigned (was: Unconfirmed)
Thank you Antonin for the investigation!

Would you like to send your patches to the DevTools?

I would rather not. Last time I tried to submit a patch it took me 2 days to setup chromium and go through the whole process. Also I'm not confident to write tests for this.

Feel free to grab my code changes and expand on them. Or take it as an inspiration and implement the fixes on your own. No worries about my fork, I will adapt to your changes when they land.

Thanks.
Labels: -Pri-2 Pri-1
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a9edbc7f8483a52d2ec12e351c4ba1ee07414ab5

commit a9edbc7f8483a52d2ec12e351c4ba1ee07414ab5
Author: lushnikov <lushnikov@chromium.org>
Date: Fri Mar 03 03:15:51 2017

DevTools: sort sourceMap mappings since they might not be ordered.

This patch stable-sorts mappings in the source map, since the spec doesn't
actually require them to be sorted.

BUG= 687772 
R=dgozman, pfeldman

Review-Url: https://codereview.chromium.org/2727383002
Cr-Commit-Position: refs/heads/master@{#454494}

[modify] https://crrev.com/a9edbc7f8483a52d2ec12e351c4ba1ee07414ab5/third_party/WebKit/LayoutTests/http/tests/inspector/text-source-map-expected.txt
[modify] https://crrev.com/a9edbc7f8483a52d2ec12e351c4ba1ee07414ab5/third_party/WebKit/LayoutTests/http/tests/inspector/text-source-map.html
[modify] https://crrev.com/a9edbc7f8483a52d2ec12e351c4ba1ee07414ab5/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js

Status: Archived (was: Assigned)
Bulk closing low-priority issues with no activity. Please re-file and refer to the closed issue if it's essential to fix.

Sign in to add a comment