NS_ERROR_FAILURE calling QueryInterface on aSubject from dom-storage2-changed notification

RESOLVED WORKSFORME

Status

()

Core
DOM
RESOLVED WORKSFORME
5 years ago
5 years ago

People

(Reporter: Ian Nartowicz, Unassigned)

Tracking

({regression})

23 Branch
x86
Linux
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox22 unaffected, firefox23 wontfix, firefox24 unaffected)

Details

Attachments

(1 attachment)

2.31 KB, application/x-xpinstall
Details
(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20100101 Firefox/17.0 (Beta/Release)
Build ID: 20130509111054

Steps to reproduce:

1. Subscribe to the observer notification dom-storage2-changed
2. Change a storage item
3. In observe(), call sSubject.QueryInterface(Ci.nsIDOMStorageEvent)


Actual results:

Exception is thrown, NS_ERROR_FAILURE


Expected results:

No exception.  The subject should be of type nsIDOMStorageEvent.  This worked until Firefox 23.

Comment 1

5 years ago
Could you attach a minimal testcase, please.
Flags: needinfo?(mozilla)
(Reporter)

Comment 2

5 years ago
Created attachment 783072 [details]
cookie-test.xpi

Trivial addon to reproduce the exception.
Flags: needinfo?(mozilla)
(Reporter)

Comment 3

5 years ago
Attached an xpi.  This is a cut-down addon which adds an observer for dom-storage2-changed, then attempts to access the resulting aSubject as nsIDOMStorageEvent.

To see the exception, install the addon (note that it is just a stripped-out Cookie Controller so it will replace that addon if you already have it) and do something that uses DOM Storage.  The Firefox start page should trigger it.  If not, go here:
http://www.quirksmode.org/html5/tests/storage.html

In Firefox 22, no error and I am able to access the StorageEvent object.  In Firefox 23, exception when calling QueryInterface(nsIDOMStorageEvent).

Updated

5 years ago
Attachment #783072 - Attachment mime type: application/octet-stream → application/x-xpinstall

Comment 4

5 years ago
Could you test with Nightly, I'm not sure I'm able to repro it with your add-on (or maybe th add-on is not compatible with FF25 and the error is not thrown).
http://nightly.mozilla.org/

Of course, I'm able to repro the issue in FF23.
Flags: needinfo?(mozilla)
(Reporter)

Comment 5

5 years ago
I can go one better.  Even the current Aurora (24a02) seems to work OK.  Apparently only FF23 is affected.
Flags: needinfo?(mozilla)
(Reporter)

Updated

5 years ago
Summary: NS_ERROR_FAILURE calling QueryInterface on aSubject from dom2-storage2-changed notification → NS_ERROR_FAILURE calling QueryInterface on aSubject from dom-storage2-changed notification
Version: 17 Branch → 23 Branch

Comment 6

5 years ago
Yes, the issue appeared in Nightly FF24, and it has been backported in Aurora FF23.
Then the issue has been fixed someway in Nightly FF24 but the fix has not been backported. That's why FF23 shows the issue, and not FF24.

STR:
1) Install the attached add-on
2) Open http://www.quirksmode.org/html5/tests/storage.html
3) Click on "setItem()" tests

Result:
Erreur : [Exception... "Failure"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://cookiecontroller/content/cookieController.jsm :: cookieControllerModule.trackStorage :: line 28"  data: no]

Regression range:
good=2013-05-16
bad=2013-05-17
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cc139752bed4&tochange=ea767da526ff

Suspected bug:
Boris Zbarsky — Bug 869195. Make QueryInterface be exposed for both chrome and xbl scopes, not just in chrome. r=bholley,peterv

Working range:
bad=2013-06-19
good=2013-06-20
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d2a7cfa34154&tochange=8ea92aeab783

Maybe fixed by:
Boris Zbarsky — Bug 884109.
Blocks: 869195
Status: UNCONFIRMED → NEW
status-firefox22: --- → unaffected
status-firefox23: --- → ?
status-firefox24: --- → unaffected
Component: Untriaged → DOM
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Hrm.  I would think if the "make QI be exposed" patch were relevant we'd go from having a QI method to not or back, not end up with a QI that throws....

The fix range has bug 847611, which is what I assume fixed this.

The bad news is that 23 is code-frozen as of a few days ago, so it'll be shipping with this bug.  I'll see if I can figure out enough about what's going on to at least suggest a workaround.

Updated

5 years ago
Depends on: 847611
OK, so I can certainly reproduce in a build from rev ea767da526ff.

We end up in mozilla::dom::QueryInterface with the "this" object being a StorageEvent, which is not a WebIDL object at that point.

I think this is what's going on: with bug 869195, QueryInterface is now no longer chromeonly, so it ends up installed via webidl quickstubs on the interfaces it's present on.  And webidl quickstubs do check whether to define things conditionally, but of course in this case (for a chrome global) the IsChromeOrXBL check on QueryInterface passes.

We had some provisions to not install QueryInterface on protos which correspond to XPConnect objects, but we did that via not installing it except on concrete interfaces that have all-abstract ancestors.

Except Event is not actually abstract in 23, even though it has xpconnect impls, because basic events are in fact on WebIDL bindings.  So we install the WebIDL quickstubs on all XPConnect prototypes of interfaces inheriting from nsIDOMEvent, and in particular StorageEvent.prototype.QueryInterface ends up being the WebIDL thing.

So as for workarounds, I think your best bet is:

  Cu.lookupMethod(event, "QueryInterface")(Ci.nsIDOMStorageEvent);

for Firefox 23.... or just removing the line entirely, since I would think it's not needed: the event should have nsIDOMStorageEvent in its classinfo.

Bobby, Olli, I don't know that there's anything to really do about this situation at this point: the only thing with webidl quickstubs is EventTarget, which is marked as abstract already, so this problem shouldn't arise again.
Flags: needinfo?(bugs)
Flags: needinfo?(bobbyholley+bmo)
(In reply to Boris Zbarsky (:bz) from comment #8)
> Bobby, Olli, I don't know that there's anything to really do about this
> situation at this point: the only thing with webidl quickstubs is
> EventTarget, which is marked as abstract already, so this problem shouldn't
> arise again.

And that's because of Window, right?

But yeah, SGTM.
Flags: needinfo?(bobbyholley+bmo)
> And that's because of Window, right?

There might be other event targets too; I haven't checked so far because there's no point worrying about it until Window is on WebIDL.

I guess I'll mark this wontfix for 23 and worksforme otherwise...
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox23: ? → wontfix
Resolution: --- → WORKSFORME
Not all event targets are webidl-fied..

But yeah, I think we need to just wontfix this :/ Too late to fix it for 23.
Flags: needinfo?(bugs)
You need to log in before you can comment on or make changes to this bug.