Last Comment Bug 782524 - window.navigator instanceof window.Window throws unexpected error
: window.navigator instanceof window.Window throws unexpected error
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla18
Assigned To: [:fabrice] Fabrice Desré
:
Mentors:
Depends on:
Blocks: system-message-api
  Show dependency treegraph
 
Reported: 2012-08-13 22:22 PDT by Sebastian Zartner [:sebo]
Modified: 2013-01-10 06:57 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
+
fixed
+
verified
+
verified


Attachments
patch (967 bytes, patch)
2012-10-01 12:15 PDT, [:fabrice] Fabrice Desré
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Sebastian Zartner [:sebo] 2012-08-13 22:22:08 PDT
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 (⌚ UTC+1/+2) 2012-08-14 04:52:47 PDT
I seem unable to reproduce on my own build from m-c changeset 22288130fea2.
Comment 2 Xiao Meng Wei :xwei 2012-08-16 10:05:57 PDT
This is still happening on today's nightly (20120816). navigator DOM object cannot be opened.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2012-10-01 09:18:47 PDT
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 [:bz] (still a bit busy) 2012-10-01 09:27:15 PDT
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 [:bz] (still a bit busy) 2012-10-01 11:34:19 PDT
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 [:bz] (still a bit busy) 2012-10-01 11:56:52 PDT
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 [:bz] (still a bit busy) 2012-10-01 12:03:54 PDT
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é 2012-10-01 12:08:16 PDT
Since we only support the API on b2g, we should not expose the interface elsewhere.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-10-01 12:09:19 PDT
OK.  You want to patch, or should I?
Comment 10 [:fabrice] Fabrice Desré 2012-10-01 12:15:14 PDT
Created attachment 666641 [details] [diff] [review]
patch
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2012-10-01 12:20:19 PDT
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] 2012-10-01 12:21:18 PDT
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 [:bz] (still a bit busy) 2012-10-01 12:22:23 PDT
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 [:bsmedberg] 2012-10-01 12:31:38 PDT
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é 2012-10-01 15:31:57 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d86a9822bd60
Comment 16 [:fabrice] Fabrice Desré 2012-10-01 15:36:33 PDT
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] 2012-10-01 16:39:06 PDT
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 19 Ed Morley [:emorley] 2012-10-02 09:00:32 PDT
https://hg.mozilla.org/mozilla-central/rev/d86a9822bd60
Comment 20 Andrew Overholt [:overholt] 2012-10-02 10:41:00 PDT
This doesn't block basecamp (if it were still open :) ).
Comment 21 Mihaela Velimiroviciu (:mihaelav) 2012-11-16 06:30:34 PST
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] 2012-11-18 13:56:42 PST
I can confirm that it's already fixed in Firefox 16.0.2. Thanks for fixing!

Sebastian
Comment 23 Paul Silaghi, QA [:pauly] 2012-12-19 02:11:35 PST
This is fixed in FF 17.0.1, but broken again on FF 18b4.
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2012-12-19 02:14:16 PST
Er...  What happened to the test I asked for in comment 11?
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2012-12-19 02:16:34 PST
That said, this worksforme on trunk.  Sounds like a branch-only issue....
Comment 26 Sebastian Zartner [:sebo] 2012-12-19 08:54:23 PST
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 [:bz] (still a bit busy) 2012-12-19 11:45:42 PST
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 [:bz] (still a bit busy) 2012-12-19 11:48:09 PST
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] 2013-01-10 06:57:31 PST
Verified fixed FF 18 based on comment 28.

Note You need to log in before you can comment on or make changes to this bug.