x-sourcemap header doesn't work

VERIFIED FIXED in Firefox 56

Status

P2
normal
VERIFIED FIXED
2 years ago
5 months ago

People

(Reporter: dave, Assigned: tromey)

Tracking

(Blocks: 1 bug)

49 Branch
Firefox 56

Firefox Tracking Flags

(firefox56 verified)

Details

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Created attachment 8796862 [details]
Screen Shot 2016-10-01 at 18.54.14.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.116 Safari/537.36

Steps to reproduce:

1. Go here: https://sourcemap-header-firefox-ienogvzqhe.now.sh/
2. Open developer tools
3. Go to inspector
4. Highlight h1 tag


Actual results:

In the right-hand css rules pane it links to main.css. It does not recognise the source map.


Expected results:

It should link to main.scss (Sass source file). Works correctly on Chrome and Safari.

Updated

2 years ago
Component: Untriaged → Developer Tools: CSS Rules Inspector
Inspector bug triage (filter on CLIMBING SHOES).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
(Assignee)

Updated

2 years ago
Blocks: 1339970
(Assignee)

Comment 2

2 years ago
Note in bug 1346936 we're adding support for the unprefixed SourceMap header as well.
(Assignee)

Updated

a year ago
Assignee: nobody → ttromey
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

a year ago
mozreview-review
Comment on attachment 8882207 [details]
Bug 1306887 - keep SourceMap response header on CSS style sheets;

https://reviewboard.mozilla.org/r/153306/#review158588

Please remember to get a DOM peer's signoff on the .webidl change.

::: dom/webidl/StyleSheet.webidl:29
(Diff revision 1)
>    readonly attribute MediaList media;
>    [Pure]
>    attribute boolean disabled;
> +  // The SourceMap response header, if seen.
> +  [ChromeOnly]
> +  readonly attribute DOMString? sourceMapURL;

Since we never return null, we can just make this "DOMString" rather than "DOMString?".

::: layout/style/Loader.cpp:889
(Diff revision 1)
> +    nsAutoCString sourceMapURL;
> +    nsresult rv = httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("SourceMap"), sourceMapURL);
> +    if (NS_FAILED(rv)) {
> +      rv = httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("X-SourceMap"), sourceMapURL);
> +    }

This "check SourceMap and if not check X-SourceMap" seems duplicated in ScriptLoader::PrepareLoadedRequest.  Can you factor that out into a helper method?

::: layout/style/StyleSheet.cpp:497
(Diff revision 1)
> +  if (mInner) {
> +    mInner->mSourceMapURL = aSourceMapURL;
> +  }

mInner should really only be null when we're unlinking the object during cycle collection.  So I think you can just go ahead and use it without null checking it.  (Same in GetSourceMapURL.)
Attachment #8882207 - Flags: review?(cam) → review+

Comment 6

a year ago
mozreview-review
Comment on attachment 8882208 [details]
Bug 1306887 - use SourceMap response header in style sheet actor;

https://reviewboard.mozilla.org/r/153308/#review158640

::: devtools/server/actors/stylesheets.js:625
(Diff revision 1)
>     *         The text of the style sheet.
>     * @return {string}
>     *         Url of source map.
>     */
>    _extractSourceMapUrl: function (content) {
> +    if (this.rawSheet.sourceMapURL) {

I think adding a comment for this return would make sense. Based on your comments, adding something like "If a SourceMap response header was saved on a style sheet, use it in the
style sheet actor."
Attachment #8882208 - Flags: review?(gl) → review+
(Assignee)

Comment 7

a year ago
(In reply to Cameron McCormack (:heycam) from comment #5)

> Please remember to get a DOM peer's signoff on the .webidl change.

I had thought you were one based on some r= I saw in StyleSheet.webidl;
but no problem, I will add another r?.

> This "check SourceMap and if not check X-SourceMap" seems duplicated in
> ScriptLoader::PrepareLoadedRequest.  Can you factor that out into a helper
> method?

I'm happy to do this but I don't know where it ought to live.
Can you suggest somewhere?
Flags: needinfo?(cam)
(In reply to Tom Tromey :tromey from comment #7)
> I had thought you were one based on some r= I saw in StyleSheet.webidl;
> but no problem, I will add another r?.

(It might have been a DOM peer requesting review of me, which might pass the commit hook.  Not sure.)

> > This "check SourceMap and if not check X-SourceMap" seems duplicated in
> > ScriptLoader::PrepareLoadedRequest.  Can you factor that out into a helper
> > method?
> 
> I'm happy to do this but I don't know where it ought to live.
> Can you suggest somewhere?

How about a static method on nsContentUtils?  Or a new NS_Blah function in nsNetUtil.h.  Either sounds fine.
Flags: needinfo?(cam)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

a year ago
Comment on attachment 8882207 [details]
Bug 1306887 - keep SourceMap response header on CSS style sheets;

Probably could use another review.
Attachment #8882207 - Flags: review+ → review?(cam)

Comment 12

a year ago
mozreview-review
Comment on attachment 8882207 [details]
Bug 1306887 - keep SourceMap response header on CSS style sheets;

https://reviewboard.mozilla.org/r/153306/#review158786

::: layout/style/Loader.cpp:890
(Diff revision 2)
> +    if (nsContentUtils::GetSourceMapURL(httpChannel.get(), sourceMapURL)) {
> +      mSheet->SetSourceMapURL(NS_ConvertUTF8toUTF16(sourceMapURL));
> +    }

(I suppose there's not much difference between checking the return value, and unconditionally calling SetSourceMapURL, since the sheet's mSourceMapURL should already be an empty string.)

::: layout/style/StyleSheet.cpp:485
(Diff revision 2)
> +NS_IMETHODIMP
> +StyleSheet::GetSourceMapURL(nsAString& aSourceMapURL)
> +{
> +  aSourceMapURL.Assign(mInner->mSourceMapURL);
> +  return NS_OK;
> +}

Can this function just return void?
Attachment #8882207 - Flags: review?(cam) → review+
(Assignee)

Comment 13

a year ago
mozreview-review-reply
Comment on attachment 8882207 [details]
Bug 1306887 - keep SourceMap response header on CSS style sheets;

https://reviewboard.mozilla.org/r/153306/#review158786

> Can this function just return void?

Yes - I've changed that.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

a year ago
mozreview-review
Comment on attachment 8882207 [details]
Bug 1306887 - keep SourceMap response header on CSS style sheets;

https://reviewboard.mozilla.org/r/153306/#review159000

r=me with the nits below.

::: dom/script/ScriptLoader.cpp:2846
(Diff revision 3)
>      if (NS_SUCCEEDED(rv) && !requestSucceeded) {
>        return NS_ERROR_NOT_AVAILABLE;
>      }
>  
>      nsAutoCString sourceMapURL;
> -    rv = httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("SourceMap"), sourceMapURL);
> +    if (nsContentUtils::GetSourceMapURL(httpChannel.get(), sourceMapURL)) {

You don't need the .get() here.  nsCOMPtr<T> auto-converts to T*.

::: dom/webidl/StyleSheet.webidl:27
(Diff revision 3)
>    readonly attribute DOMString? title;
>    [Constant]
>    readonly attribute MediaList media;
>    [Pure]
>    attribute boolean disabled;
> +  // The SourceMap response header, if seen.

What if not seen?  Does it return empty string?  Something else?

Assuming we want to treat "sent, but empty string" the same as "not sent", just using empty string to represent "not sent" is fine.  But please document.

Also might be worth a comment about how it can change once we actually get back the HTTP response, so is not [Constant].  But it should probably be marked [Pure].

::: layout/style/Loader.cpp:890
(Diff revision 3)
>        mLoader->SheetComplete(this, NS_ERROR_NOT_AVAILABLE);
>        return NS_OK;
>      }
> +
> +    nsAutoCString sourceMapURL;
> +    if (nsContentUtils::GetSourceMapURL(httpChannel.get(), sourceMapURL)) {

No need for .get().

::: layout/style/StyleSheet.cpp:488
(Diff revision 3)
>  }
>  
> +void
> +StyleSheet::GetSourceMapURL(nsAString& aSourceMapURL)
> +{
> +  aSourceMapURL.Assign(mInner->mSourceMapURL);

Just `aSourceMapURL = mInner->mSourceMapURL`.

::: layout/style/StyleSheetInfo.h:64
(Diff revision 3)
>    // its hands on a child sheet that means we've already ensured unique infos
>    // throughout its parent chain and things are good.
>    RefPtr<StyleSheet>     mFirstChild;
>    AutoTArray<StyleSheet*, 8> mSheets;
>  
> +  // Source map URL from the SourceMap response header, if given.

Again, document what happens if the header is not present.
Attachment #8882207 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 17

a year ago
I've addressed the review comments.  Also I had to update the test to use async/await
in order for it to work after rebasing.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

a year ago
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a97c16caf744
keep SourceMap response header on CSS style sheets; r=bz,heycam
https://hg.mozilla.org/integration/autoland/rev/7023101ff34e
use SourceMap response header in style sheet actor; r=gl

Comment 21

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a97c16caf744
https://hg.mozilla.org/mozilla-central/rev/7023101ff34e
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Comment 22

a year ago
I have reproduced this Bug on Nightly 52.0a1 (2016-10-01) on Windows 10, 64 bit!

The bug's fix is now verified on latest  Nightly 56.0a1

Build ID    :	20170712030204
User Agent  : 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
QA Whiteboard: [bugday-20170712]

Updated

a year ago
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified

Updated

5 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.