Closed Bug 871887 Opened 11 years ago Closed 11 years ago

marquee's _setEventListener does not work

Categories

(Core :: Layout, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox21 + wontfix
firefox22 + verified
firefox23 + fixed
firefox24 + fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: moz_bug_r_a4, Assigned: bholley)

References

Details

(Keywords: regression, sec-other, Whiteboard: [adv-main22-])

Attachments

(4 files)

Attached file testcase
This is a regression from bug 865947. Filing as security-sensitive just in case.

marquee's _setEventListener does not work because

new XPCNativeWrapper.unwrap(window).Function("event", aValue);

causes the following error:
TypeError: XPCNativeWrapper.unwrap is not a constructor
Assignee: nobody → bobbyholley+bmo
Blocks: 865947
Are we about to ship this web compat regression in 21?  :(  Wish we had test coverage for marquee...
Keywords: regression
Depends on: 872273
Marking as sec-other because there's no known problem beyond breaking marquee.  Please feel free to change to high or whatever as appropriate.
Keywords: sec-other
Security concerns are secondary, so renominating. Can somebody go into specifics about the user impact here?
The user impact is that scripts manipulating marquee will be broken on some sites.

In practice, I believe this is mostly a problem in China; marquee is pretty much unused elsewhere.
Depends on: 872772
So, at first I had a mind to write a bunch of extensive test coverage for this stuff, and converge with the spec. But as I started digging into the various quirks of our implementation, I started to get concerned about all the ominous IE6 compat comments, and realized that this isn't the most worthwhile battle for me to fight right now. So I'll just fix this bug.

In fixing this I also ran into bug 872772. I'm fixing that separately, but given that this is going to need to be backported, I'm going to trivially work around it in this bug.
No longer depends on: 872273, 872772
Attachment #750202 - Flags: review?(bzbarsky)
Comment on attachment 750202 [details] [diff] [review]
Fix marquee _setEventListener. v1

r=me
Attachment #750202 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #4)
> The user impact is that scripts manipulating marquee will be broken on some
> sites.

Please let us know if there are any known websites that may be broken.
> 
> In practice, I believe this is mostly a problem in China; marquee is pretty
> much unused elsewhere.

Based on above, I am ccing Hong Tang from Moz China to see if they have seen/heard of any web-compat regressions in support forums or via bug reports.
I am tracking this as this is a web-compat regression.At this point I don't think this is a driver for 21.0.1 or a ride-along even(to avoid possibility of new fallouts). I want to get more information on how many real world websites may be impacted and will "wontfix" accordingly for Fx21 or see what the next best step may be.

When the patch is ready we should already be uplifting this to other branches with clear risk evaluation & testing needed or completed.
Comment on attachment 750202 [details] [diff] [review]
Fix marquee _setEventListener. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 865947
User impact if declined: <marquee onfoo=""> listeners are broken
Testing completed: Just pushed to m-i.
Risk to taking this patch (and alternatives if risky): Not risky. The lines it changes are currently just straight up broken (that is to say, they just throw) in the builds we ship. We would have noticed if we had any test coverage of marquee at all.
String or UUID changes made by this patch: None
Attachment #750202 - Flags: approval-mozilla-esr17?
Attachment #750202 - Flags: approval-mozilla-beta?
Attachment #750202 - Flags: approval-mozilla-b2g18?
Attachment #750202 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/0ce36946c295
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment on attachment 750202 [details] [diff] [review]
Fix marquee _setEventListener. v1

Approving for all affected branches. Is FF17/18 really affected?
Attachment #750202 - Flags: approval-mozilla-beta?
Attachment #750202 - Flags: approval-mozilla-beta+
Attachment #750202 - Flags: approval-mozilla-aurora?
Attachment #750202 - Flags: approval-mozilla-aurora+
(In reply to Alex Keybl [:akeybl] from comment #13)
> Comment on attachment 750202 [details] [diff] [review]
> Fix marquee _setEventListener. v1
> 
> Approving for all affected branches. Is FF17/18 really affected?

They are not, because we didn't land bug 865947 there. Sorry for the confusion.
Attachment #750202 - Flags: approval-mozilla-esr17?
Attachment #750202 - Flags: approval-mozilla-b2g18?
It seems that there is a compat regression.  When onfoo handler is called, |this| should refer to event.target, but |this| refers to the window on trunk.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached file testcase 2
Comment on attachment 751733 [details] [diff] [review]
Addendum: Make sure |this|-binding is correct or marquee event listeners. v1

r=me.  Really wish we could stop faking this stuff and just do C++ event handlers.  :(
Attachment #751733 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/41c950a2f7db

(Note that this addendum should be uplifted together with the original patch)
https://hg.mozilla.org/mozilla-central/rev/41c950a2f7db
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Verified as fixed on Mozilla/5.0 (Windows NT 5.1; rv:22.0) Gecko/20100101 Firefox/22.0 (20130528181031).
I filed bug 880425 on killing off this code completely.
Whiteboard: [adv-main22-]
Group: core-security
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.