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

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect)

Description

[Ian Nartowicz]

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

[Loic]

Could you attach a minimal testcase, please.

Comment 2

[Ian Nartowicz]

Attachment: cookie-test.xpi

Trivial addon to reproduce the exception.

Comment 3

[Ian Nartowicz]

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

Comment 4

[Loic]

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.

Comment 5

[Ian Nartowicz]

I can go one better.  Even the current Aurora (24a02) seems to work OK.  Apparently only FF23 is affected.

Comment 6

[Loic]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Bobby Holley (:bholley)]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Not all event targets are webidl-fied..

But yeah, I think we need to just wontfix this :/ Too late to fix it for 23.