1020846 - Unexpected Error messages: "[...] is being assigned a //# sourceMappingURL, but already has one"

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[max.limper]

Attachment: A minimalistic example that reproduces the error

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.114 Safari/537.36

Steps to reproduce:

I'm using TypeScript.

All "transpiled" foo.js files are placed right next to the corresponding foo.ts files, and along with a foo.js.map file for each of them. My IDE (WebStorm 8.0.2) generates the .js and .js.map files automatically. Each .js file contains exactly one time a line like the following:
//# sourceMappingURL=foo.js.map


Actual results:

For each js file in my project, Firefox (29.0.1) outputs an error message like the following to the console:

Error: http://localhost:8000/foo.js is being assigned a //# sourceMappingURL, but already has one



Expected results:

As, to the best of my knowledge, the sourceMappingURL is defined only once, there shouldn't be any error message.
Chrome (35.0.1916.114 m) does not output any errors.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I don't see any warnings or errors when loading the attached test.html in Firefox 29...

Do you see the issue in a clean profile with no addons?

Comment 2

[max.limper]

Attachment: Screenshot of the error message

Comment 3

[max.limper]

Yes, I just uploaded a screenshot.
I have re-installed Firefox to make sure, with the latest version, and without any plugins installed, I still get the error.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm.  I'm loading the test.html file via file://, not http://.  Does that make a difference?

Comment 5

[max.limper]

Indeed, it does. If I load it via "file://", the Error message does not pop up.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ah, I see what's going on here.  The patch for bug 765993 did this:

    nsAutoCString sourceMapURL;
    httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("X-SourceMap"), sourceMapURL);
    aRequest->mHasSourceMapURL = true;
    aRequest->mSourceMapURL = NS_ConvertUTF8toUTF16(sourceMapURL);

which means it will then always pass an empty string for the source map URL via compile options.

Eddy, is there a reason we're not setting mHasSourceMapURL to whether GetResponseHeader() succeeded or something?

Comment 7

[Andrew McCreight [:mccr8]]

FWIW, this error also happens while running Mochitest chrome on content/base/test/chrome/test_bug765993.html.  I don't know if that is expected or not.

Comment 8

[Andrew McCreight [:mccr8]]

Oh and I think we leak when we hit that condition.  See bug 1022954.

Comment 9

[Eddy Bruel [:ejpbruel]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> Ah, I see what's going on here.  The patch for bug 765993 did this:
> 
>     nsAutoCString sourceMapURL;
>     httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("X-SourceMap"),
> sourceMapURL);
>     aRequest->mHasSourceMapURL = true;
>     aRequest->mSourceMapURL = NS_ConvertUTF8toUTF16(sourceMapURL);
> 
> which means it will then always pass an empty string for the source map URL
> via compile options.
> 
> Eddy, is there a reason we're not setting mHasSourceMapURL to whether
> GetResponseHeader() succeeded or something?

My memory is vague on this, but I seem to recall that (In reply to Boris Zbarsky [:bz] from comment #6)
> Ah, I see what's going on here.  The patch for bug 765993 did this:
> 
>     nsAutoCString sourceMapURL;
>     httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("X-SourceMap"),
> sourceMapURL);
>     aRequest->mHasSourceMapURL = true;
>     aRequest->mSourceMapURL = NS_ConvertUTF8toUTF16(sourceMapURL);
> 
> which means it will then always pass an empty string for the source map URL
> via compile options.
> 
> Eddy, is there a reason we're not setting mHasSourceMapURL to whether
> GetResponseHeader() succeeded or something?

Looks like a bug to me. I didn't consider the possibility that GetResponseHeader might fail.

Comment 10

[Le Roux Bodenstein]

I ran into this in Firefox 29 and now also Firefox 30. I have exactly one source mapping defined (I checked and double-checked my output) and firefox throws this as an error on every page hit.

For what it is worth: I'm using brunch (http://brunch.io/) to compile, concatenate and minify my files, so this is all automated.

Doesn't look like there's any way for devs to fix this on our ends, so I'm left with a decision between errors or no source maps.

(Also: Shouldn't this kind of thing lead to a warning at most and not an error?)

Comment 11

[Boris Zbarsky [:bzbarsky]]

For what it's worth this _is_ a warning.  The text contains "Error:" because the JS engine is being dumb, but the message category is warning, and you will only see it in your console if you have filters set there to show warnings.

Comment 12

[Andrew McCreight [:mccr8]]

Without a fix for this, it looks like fixing the leaks causes various tests to time out, presumably because some exception gets thrown in a way that messes up the test.

The tests that break are:
browser/devtools/debugger/test/browser_dbg_blackboxing-01.js
browser/devtools/debugger/test/browser_dbg_blackboxing-05.js
browser/devtools/debugger/test/browser_dbg_pretty-print-09.js
browser/devtools/debugger/test/browser_dbg_source-maps-01.js
browser/devtools/debugger/test/browser_dbg_source-maps-02.js
browser/devtools/debugger/test/browser_dbg_source-maps-03.js

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: Don't say we have a url if GetResponseHeaderFails.

This seemed to be what bz was suggesting, not that I entirely understand this code.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Pretty much (though we should also CopyUTF8toUTF16 while we're here and avoid the extra string object.

Comment 15

[Till Schneidereit [:till]]

Eddy, is this on your radar? It's pretty annoying when working with TypeScript projects with lots of files, such as Shumway.

Comment 16

[Eddy Bruel [:ejpbruel]]

(In reply to Till Schneidereit [:till] from comment #15)
> Eddy, is this on your radar? It's pretty annoying when working with
> TypeScript projects with lots of files, such as Shumway.

Hey Till. It's still on my radar, but wasn't very high priority until now. I'll put it on top of my todo list for next week, since you're the one that asked.

Comment 17

[Till Schneidereit [:till]]

Very kind, good sir! Please don't let it get in the way of your process, though: I have lived with this for a while now, and will be able to do so for a little while longer.

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

FWIW, this won't be reported in the console as an "error" despit being a warning anymore: bug 1027157

Comment 19

[Alex Henrie]

Comment on attachment 8440009 [details] [diff] [review]
Don't say we have a url if GetResponseHeaderFails.

I'm trying to push this along because this bug appears to be causing Selenium tests to freeze Firefox at random.

Comment 20

[Alex Henrie]

Attachment: backtrace from frozen Firefox

When I try to run Selenium tests, Firefox will freeze at random (no particular test causes Firefox to freeze, but it always freezes _somewhere_). I attached GDB to the frozen process and obtained the following backtrace, which led me to this bug report.

Comment 21

[Henri Sivonen (:hsivonen)]

Comment on attachment 8440009 [details] [diff] [review]
Don't say we have a url if GetResponseHeaderFails.

Review of attachment 8440009 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsScriptLoader.cpp
@@ +1480,5 @@
>  
>      nsAutoCString sourceMapURL;
> +    rv = httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("X-SourceMap"), sourceMapURL);
> +    if (NS_FAILED(rv)) {
> +      aRequest->mHasSourceMapURL = false;

mHasSourceMapURL is always initialized to false, AFAICT.

Unless there's a reason that I'm missing to assign false here, I suggest not having this |if| branch and make the whole |if| read:

if (NS_SUCCEEDED(rv)) {
  aRequest->mHasSourceMapURL = true;
  aRequest->mSourceMapURL = NS_ConvertUTF8toUTF16(sourceMapURL);
}

r+ with that.

Comment 22

[Eddy Bruel [:ejpbruel]]

(In reply to Alex Henrie from comment #19)
> Comment on attachment 8440009 [details] [diff] [review]
> Don't say we have a url if GetResponseHeaderFails.
> 
> I'm trying to push this along because this bug appears to be causing
> Selenium tests to freeze Firefox at random.

Thank you for taking this off my plate. I had intended to write a patch last week in response to Till's comment but I got preoccupied by other stuff and it slipped of my radar again.

Till, sorry for not living up to my promise on this one!

Comment 23

[Alex Henrie]

Attachment: Don't say we have a url if GetResponseHeaderFails.

(In reply to Henri Sivonen (:hsivonen) from comment #21)
> mHasSourceMapURL is always initialized to false, AFAICT.

I think you're right. Good point.

Comment 24

[Alex Henrie]

Requesting checkin of attachment 8453942 [details] [diff] [review]

Henri Sivonen approved this patch in comment #21.

Comment 25

[Andrew McCreight [:mccr8]]

This still needs a try run, and I want to look it over again.  I'll do that today.

Comment 26

[Andrew McCreight [:mccr8]]

try run: https://tbpl.mozilla.org/?tree=Try&rev=04bde7a2f313
(the M3 failure is unrelated)

Thanks for moving this along, Alex.  Once it lands, I'll try to get it on Aurora so the fix will be released a little sooner.  (We're too late for Beta at this point, as it is going to be released very soon.)

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8acd862a180f

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8acd862a180f

Comment 29

[Andrew McCreight [:mccr8]]

Comment on attachment 8453942 [details] [diff] [review]
Don't say we have a url if GetResponseHeaderFails.

Approval Request Comment
[Feature/regressing bug #]: bug 765993
[User impact if declined]: annoying errors for webdevs, some kind of freezing in Selenium testing (see comment 20)
[Describe test coverage new/current, TBPL]: there's existing code that runs this on TBPL
[Risks and why]: low risk, just does a minor change to one odd case
[String/UUID change made/needed]: none

Comment 30

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8453942 [details] [diff] [review]
Don't say we have a url if GetResponseHeaderFails.

Let's take this small fix to improve the lives of web devs. Aurora+

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f599b01ed454