782524 - window.navigator instanceof window.Window throws unexpected error

Status: VERIFIED FIXED

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

Description

[Sebastian Zartner [:sebo]]

Executing "window.navigator instanceof window.Window" or checking against other globally defined objects like window.Document or window.Node causes an unexpected error.

This error occurs within the current (2012-08-13) Firefox Nightly and Aurora build. It does not happen in the latest Beta or 14.0.1.

Test case:
1. Open the Web Console
2. Type "window.navigator instanceof window.Window" (without quotes) and hit Enter

Sebastian

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I seem unable to reproduce on my own build from m-c changeset 22288130fea2.

Comment 2

[Xiao Meng Wei :xwei]

This is still happening on today's nightly (20120816). navigator DOM object cannot be opened.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a31fc9052840&tochange=e61399f31505 based on nightlies.

But my debug build seems to not be showing the problem???

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, something is _really_ wacky here.  Today's nightly shows the problem for me, but neither the debug nor opt build I made last night from a clean tree show the problem...

Marking security-sensitive, since my best guess is that something is reading memory it should not somewhere.

Comment 5

[Boris Zbarsky [:bzbarsky]]

A debug try build says:

###!!! ASSERTION: nsDOMConstructor::HasInstance can't get interface info.: 'Error', file ../../../dom/base/nsDOMClassInfo.cpp, line 6391

so perhaps something is not being packaged that should be?

Comment 6

[Boris Zbarsky [:bzbarsky]]

6459        iim->GetInfoForIID(class_interface, getter_AddRefs(if_info));
6460        if (!if_info) {
6461          NS_ERROR("nsDOMConstructor::HasInstance can't get interface info.");
6462          return NS_ERROR_UNEXPECTED;
6463        }

(gdb) p/x *class_interface
$2 = {
  m0 = 0x91e90dd, 
  m1 = 0xe8b, 
  m2 = 0x463d, 
  m3 = {0x8c, 0xdc, 0x92, 0x25, 0xd3, 0xa6, 0xff, 0x90}
}

That's the IID for nsIDOMNavigatorSystemMessages.  Is the xpt for that not being packaged?  But why is that the class_interface here?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, because we check all the classinfo interfaces in that loop.

So yeah, it looks like we only package dom_messages.xpt on b2g, but expose the interface everywhere?  That's very broken.

We need to fix this on beta before we ship Fx16 next week...  Not sure whether the fix is to ship the xpt or to not expose the interface; over to Fabrice to decide that.

Comment 8

[[:fabrice] Fabrice Desré]

Since we only support the API on b2g, we should not expose the interface elsewhere.

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK.  You want to patch, or should I?

Comment 10

[[:fabrice] Fabrice Desré]

Attachment: patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 666641 [details] [diff] [review]
patch

r=me, especially if you add a mochitest for the original testcase in this bug (just checking that it returns false)... ;)

Comment 12

[Alex Keybl [:akeybl]]

Sounds like this will be very low risk (assuming there's no IID change with add-on compat impact), but I want to make sure we need to actually make a change here. We haven't heard user complaints related to this, so what's the risk to leaving this in? Long term issue with the availability of this interface, once people start actually using it?

Today (EOD) will be our final beta build for FF16.

Comment 13

[Boris Zbarsky [:bzbarsky]]

The user-facing issue is that some scripts will throw exceptions when they should not.  Whether this breaks sites in practice is an open question, of course.

Comment 14

[Benjamin Smedberg]

This seems like something which could easily be hit in the wild and could potentially cause a dot-release if we didn't fix it, and I believe we should take the fix.

Comment 15

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d86a9822bd60

Comment 16

[[:fabrice] Fabrice Desré]

Comment on attachment 666641 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Exposing an interface to classInfo but not providing implementation.
User impact if declined: Some valid code is throwing exceptions without this patch
Risk to taking this patch (and alternatives if risky): No risk. 
String or UUID changes made by this patch: None

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 666641 [details] [diff] [review]
patch

[Triage Comment]
Approving for branches as well. Please land asap to make our last beta build today.

Comment 18

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1bdd0bd4dd1d
https://hg.mozilla.org/releases/mozilla-beta/rev/8a8a932fbef4

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d86a9822bd60

Comment 20

[Andrew Overholt [:overholt]]

This doesn't block basecamp (if it were still open :) ).

Comment 21

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0
Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/17.0 Firefox/17.0

Verified the fix on the latest Firefox 17.0 beta builds (beta 6) and the exception doesn't appear anymore.
Marking verified for Firefox 17.

Comment 22

[Sebastian Zartner [:sebo]]

I can confirm that it's already fixed in Firefox 16.0.2. Thanks for fixing!

Sebastian

Comment 23

[Paul Silaghi, QA [:pauly]]

This is fixed in FF 17.0.1, but broken again on FF 18b4.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Er...  What happened to the test I asked for in comment 11?

Comment 25

[Boris Zbarsky [:bzbarsky]]

That said, this worksforme on trunk.  Sounds like a branch-only issue....

Comment 26

[Sebastian Zartner [:sebo]]

I can confirm the observations.
It for me on FF 17.0.1/19.0a2 (2012-12-18)/20.0a1 (2012-12-19), but is broken in the lastest 18.0 beta.

Sebastian

Comment 27

[Boris Zbarsky [:bzbarsky]]

OK, I can reproduce on beta.  The issue there is nsIDOMNavigatorTime.

Let's get a separate bug filed on that, since people apparently went and regressed that and it's a different issue from this bug, though related.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Filed bug 823173.  But we still need a test here!  I guess we can add it in the other bug...

Comment 29

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 18 based on comment 28.