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)
Core Graveyard
Plug-ins
Tracking
(firefox22+ wontfix, firefox23+ fixed, firefox24+ fixed)
RESOLVED
FIXED
mozilla24
People
(Reporter: gk, Assigned: bzbarsky)
References
Details
(Keywords: addon-compat)
Attachments
(3 files, 1 obsolete file)
2.06 KB,
application/x-xpinstall
|
Details | |
2.50 KB,
application/javascript
|
Details | |
8.77 KB,
patch
|
johns
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•12 years ago
|
Attachment #763683 -
Attachment mime type: application/octet-stream → application/x-xpinstall
Updated•12 years ago
|
Component: DOM → Plug-ins
Comment 1•12 years ago
|
||
What does "replacing notificationCallbacks" mean in this context?
Comment 2•12 years ago
|
||
Reporter | ||
Comment 3•12 years ago
|
||
(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.
![]() |
Assignee | |
Comment 4•12 years ago
|
||
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.
![]() |
Assignee | |
Comment 5•12 years ago
|
||
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
![]() |
Assignee | |
Comment 6•12 years ago
|
||
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.
Updated•12 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Hardware: x86 → All
![]() |
Assignee | |
Comment 7•12 years ago
|
||
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)
![]() |
Assignee | |
Comment 8•12 years ago
|
||
Reporter | ||
Comment 9•12 years ago
|
||
http://www.twitch.tv/ (that got buried in my bug description, sorry).
Flags: needinfo?(gk)
![]() |
Assignee | |
Comment 10•12 years ago
|
||
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)
![]() |
Assignee | |
Comment 11•12 years ago
|
||
Attachment #764343 -
Flags: review?(jschoenick)
![]() |
Assignee | |
Updated•12 years ago
|
Attachment #763754 -
Attachment is obsolete: true
Attachment #763754 -
Flags: review?(jschoenick)
Comment 12•12 years ago
|
||
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+
Comment 13•12 years ago
|
||
CC stuff looks okay to me.
![]() |
Assignee | |
Comment 14•12 years ago
|
||
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...
![]() |
Assignee | |
Updated•12 years ago
|
Flags: in-testsuite?
Keywords: addon-compat
![]() |
Assignee | |
Comment 15•12 years ago
|
||
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?
![]() |
Assignee | |
Comment 16•12 years ago
|
||
> User impact if declined: Some extensions will cause plug-ins to not work at all.
"In some cases". Probably involving redirects...
Updated•12 years ago
|
Updated•12 years ago
|
QA Contact: jbecerra
Comment 17•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Updated•12 years ago
|
Attachment #764343 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 18•12 years ago
|
||
Updated•3 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•