Replacing notificationCallbacks prevents plugin content from getting loaded

RESOLVED FIXED in Firefox 23

Status

()

RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: gk, Assigned: bzbarsky)

Tracking

({addon-compat})

Trunk
mozilla24
addon-compat
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

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

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 763683 [details]
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
(Reporter)

Updated

5 years ago
Attachment #763683 - Attachment mime type: application/octet-stream → application/x-xpinstall
(Reporter)

Updated

5 years ago
Blocks: 827158
OS: Windows 7 → All
Component: DOM → Plug-ins

Comment 1

5 years ago
What does "replacing notificationCallbacks" mean in this context?
Created attachment 763693 [details]
The extension JS in question
(Reporter)

Comment 3

5 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

5 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

5 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

5 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.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Hardware: x86 → All
(Assignee)

Comment 7

5 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)
(Reporter)

Comment 9

5 years ago
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)
Created attachment 764343 [details] [diff] [review]
And with a test fix based on try run
Attachment #764343 - Flags: review?(jschoenick)
(Assignee)

Updated

5 years ago
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...
status-firefox22: --- → wontfix
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?
(Assignee)

Updated

5 years ago
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...

Updated

5 years ago
status-firefox23: --- → affected
status-firefox24: --- → affected
tracking-firefox22: --- → +
tracking-firefox23: ? → +
tracking-firefox24: ? → +

Updated

5 years ago
QA Contact: jbecerra
https://hg.mozilla.org/mozilla-central/rev/798c709e591f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox24: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Attachment #764343 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment hidden (spam)
You need to log in before you can comment on or make changes to this bug.