Closed Bug 883968 Opened 10 years ago Closed 10 years ago

Replacing notificationCallbacks prevents plugin content from getting loaded


(Core Graveyard :: Plug-ins, defect)

Not set


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

Tracking Status
firefox22 + wontfix
firefox23 + fixed
firefox24 + fixed


(Reporter: gk, Assigned: bzbarsky)



(Keywords: addon-compat)


(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 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
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.

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.
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 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 (that got buried in my bug description, sorry).
Flags: needinfo?(gk)
Comment on attachment 763754 [details] [diff] [review]

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.

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
Closed: 10 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.