Closed Bug 883968 Opened 12 years ago Closed 12 years ago

Replacing notificationCallbacks prevents plugin content from getting loaded

Categories

(Core Graveyard :: Plug-ins, defect)

defect
Not set
normal

Tracking

(firefox22+ wontfix, firefox23+ fixed, firefox24+ fixed)

RESOLVED FIXED
mozilla24
Tracking Status
firefox22 + wontfix
firefox23 + fixed
firefox24 + fixed

People

(Reporter: gk, Assigned: bzbarsky)

References

Details

(Keywords: addon-compat)

Attachments

(3 files, 1 obsolete file)

Attached file test.xpi
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/20100101 Firefox/17.0 (Beta/Release) Steps to reproduce: I opened twitch.tv having a (test) extension (which is attached) in a clean new profile both in a stable Firefox (21.0) and the recent beta (22.0b5). Actual results: The flash content gets loaded in the stable but not the beta and later versions. Expected results: The flash content should get loaded in 22.0b5 and later versions as well. I bisected that problem down to https://hg.mozilla.org/mozilla-central/rev/f22f257f7330
Attachment #763683 - Attachment mime type: application/octet-stream → application/x-xpinstall
Blocks: 827158
OS: Windows 7 → All
Component: DOM → Plug-ins
What does "replacing notificationCallbacks" mean in this context?
(In reply to Benjamin Smedberg [:bsmedberg] from comment #1) > What does "replacing notificationCallbacks" mean in this context? Doing things like this.origCbs = httpChannel.notificationCallbacks; httpChannel.notificationCallbacks = new NotCallbackTester(this.origCbs); and NotCallbackTester could then e.g. provide an own implementation of nsIAuthPromptProvider and friends while giving back the original notification callbacks if other interfaces are requested.
Exciting. So nsObjectLoadingContent::OpenChannel passes "this" as the notification callbacks. But of course the JS object no longer implements nsIInterfaceRequestor, so the this.originalNotificationCallbacks.getInterface(aIID) thing in the attached extension would throw. The extension then swallows the exception, which probably didn't make debugging that any more fun. We fixed this for XMLHttpRequest in bug 780529. I guess we need something like that here.
So I'm going to do a simpler change here: just a shim interface requestor that forwards nsIChannelEventSink to the object loading content.
Assignee: nobody → bzbarsky
And we'll hope that no one assumes they can get the node from the channel via the notification callbacks here here... If people _do_ assume that, we need something more like the XHR fix.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Hardware: x86 → All
OK, I have a patch.... but I can't reproduce this bug. If I install this extension and go to http://www.speakeasy.net/speedtest/ the plug-in is instantiated just fine. Georg, what's a url that shows the problem?
Flags: needinfo?(gk)
Attached patch Patch (obsolete) — Splinter Review
http://www.twitch.tv/ (that got buried in my bug description, sorry).
Flags: needinfo?(gk)
Comment on attachment 763754 [details] [diff] [review] Patch Thanks, I can reproduce there and the patch fixes it.
Attachment #763754 - Attachment description: Patch, untested so far → Patch
Attachment #763754 - Flags: review?(jschoenick)
Attachment #764343 - Flags: review?(jschoenick)
Attachment #763754 - Attachment is obsolete: true
Attachment #763754 - Flags: review?(jschoenick)
Comment on attachment 764343 [details] [diff] [review] And with a test fix based on try run Review of attachment 764343 [details] [diff] [review]: ----------------------------------------------------------------- With the caveat that I'm not very familiar with our XPCOM or cycle collector macros, this lgtm
Attachment #764343 - Flags: review?(jschoenick) → review+
CC stuff looks okay to me.
https://hg.mozilla.org/integration/mozilla-inbound/rev/798c709e591f Unfortunately, this was reported too late to make it into 22, but I think we should backport to 23...
Flags: in-testsuite?
Keywords: addon-compat
Comment on attachment 764343 [details] [diff] [review] And with a test fix based on try run [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 827158 User impact if declined: Some extensions will cause plug-ins to not work at all. Testing completed (on m-c, etc.): Passes the testcase in the bug. Risk to taking this patch (and alternatives if risky): Risk is medium to low. The other alternative would be disabling WebIDL bindings for <object>, which is probably more risky. String or IDL/UUID changes made by this patch: None.
Attachment #764343 - Flags: approval-mozilla-aurora?
> User impact if declined: Some extensions will cause plug-ins to not work at all. "In some cases". Probably involving redirects...
QA Contact: jbecerra
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Attachment #764343 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: