Closed Bug 1282332 Opened 3 years ago Closed 3 years ago

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox47 --- unaffected
firefox48 --- unaffected
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: birtles, Assigned: mrrrgn)

References

()

Details

(Keywords: regression)

Attachments

(1 file)

See: https://github.com/webcompat/web-bugs/issues/2797

Loading the bug URL in Firefox 49 or later shows a page that gets stuck loading content. It says, ただいま地図を読み込み中です... ("Currently downloading map...").

In the console there is an unhandled exception which seems to be related to the AJAX request (based on the stack): "TypeError: str.match(...) is null"

mozregression points to bug 1266255 as the cause.

Loading the same URL in Chrome/Edge/Firefox 48 or less, displays the content as expected.
Morgan, do you know what is happening here?
Flags: needinfo?(winter2718)
I see the error, we're incorrectly deriving the function's name from it's display atom: ".Deferred/</i[o[0]]()" to "0]" when it should be "".
Flags: needinfo?(winter2718)
Assignee: nobody → winter2718
Attached patch displayname.diffSplinter Review
This is so ... yuck :( I'm going to see about ripping this entire "display name parsing" mechanism when I work on computed property names.
Attachment #8765309 - Flags: review?(jorendorff)
Comment on attachment 8765309 [details] [diff] [review]
displayname.diff

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

Ugh. Maybe it is time to throw away displayName.
Attachment #8765309 - Flags: review?(jorendorff) → review+
Pushed by mphillips@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b85f918f0db
Refuse to parse display names with unquoted, non-numeric, property names; r=jorendorff
(In reply to Jason Orendorff [:jorendorff] from comment #4)
> Comment on attachment 8765309 [details] [diff] [review]
> displayname.diff
> 
> Review of attachment 8765309 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ugh. Maybe it is time to throw away displayName.

Maybe I can do all of this with a new opcode: JSOP_SETFNAME (or something along those lines). If not, yes, I'll open a bug to get a dialogue started with devtools. It's a real thorn! <sub>ps, thx for the quick review. :]</sub>
s/I/we/g
https://hg.mozilla.org/mozilla-central/rev/9b85f918f0db
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Thanks for the quick fix! We'll want to uplift this to Aurora once it's baked on trunk.
Hi Morgan, thanks for fixing this so quickly. Would you mind requesting aurora approval? Thanks!
Flags: needinfo?(winter2718)
(In reply to Brian Birtles (:birtles) from comment #10)
> Hi Morgan, thanks for fixing this so quickly. Would you mind requesting
> aurora approval? Thanks!

Derp! Thanks for the reminder, and for reporting this! :)
Flags: needinfo?(winter2718)
Comment on attachment 8765309 [details] [diff] [review]
displayname.diff

Approval Request Comment
[Feature/regressing bug #]: 1282332
[User impact if declined]: Broken web pages.
[Describe test coverage new/current, TreeHerder]: Tests are included with the patch.
[Risks and why]: Risks are minimal. The changes are isolated.
[String/UUID change made/needed]:
Attachment #8765309 - Flags: approval-mozilla-aurora?
Comment on attachment 8765309 [details] [diff] [review]
displayname.diff

Fix for regression from 49, includes tests, let's take it!
Attachment #8765309 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.