Closed Bug 1442344 Opened 2 years ago Closed 2 years ago

Make it clearer that WrapObject does not get called on globals (like windows)

Categories

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

enhancement
Not set

Tracking

()

RESOLVED INVALID
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

Most globals just have their WrapObject() assert/crash.  Windows should too.
MozReview-Commit-ID: Hq2xA1UetQu
Attachment #8955238 - Flags: review?(kyle)
Attachment #8955238 - Flags: review?(kyle) → review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b89286a88588
Make it clearer that nsGlobalWindowInner::WrapObject is never called.  r=qdot
https://hg.mozilla.org/mozilla-central/rev/b89286a88588
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Backed out for frequently failing crashtests on android, e.g. libeditor/crashtests/772282.html and layout/generic/crashtests/542136-1.html

Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b89286a885886784f8fa728e2652bed07362935c

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=165537196&repo=mozilla-inbound&lineNumber=1894

Backout: https://hg.mozilla.org/mozilla-central/rev/accb1b3cf5932526b4cf983819f9fb4cc1111546
Flags: needinfo?(bzbarsky)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla60 → ---
Are you sure it was this patch?  This patch explicitly puts a MOZ_CRASH on a codepath.  If we were hitting that codepath at all (which we shouldn't be), I would expect to see that MOZ_CRASH message in the log (which I don't).
Flags: needinfo?(apavel)
Boris, hi. No, sorry, i had both bugs opened. The other patch is the one with the issue, will update the bugs shortly.
Flags: needinfo?(apavel)
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Flags: needinfo?(bzbarsky)
Ah, the "extra" log does have a MozCrashReason.  Need to figure out why we're getting there, but the stack in the logs is useless.  :(
Hold on.  We are clearly hitting a "We should never get here!" MOZ_CRASH, which is very likely to be this bug, and not at all likely to be bug 1442313.  Furthermore, the tests involved don't even use workers, which is all bug 1442313 touched.

So you _should_ be backing out this bug and should _not_ be backing out bug 1442313.
Flags: needinfo?(apavel)
Boris: the failure log is this: https://treeherder.mozilla.org/logviewer.html#?job_id=165537196&repo=mozilla-inbound&lineNumber=1894

I searched for MOZ_CRASH in that log and did not find anything. Could you tell me what i'm missing? 

Aryx: can you help clarify this?
Flags: needinfo?(apavel) → needinfo?(aryx.bugmail)
> Could you tell me what i'm missing? 

Android is dump.  The MOZ_CRASH is in the .extra file, not in the log:

  MozCrashReason=MOZ_CRASH(We should never get here!)
Flags: needinfo?(apavel)
> Android is dump.

"dumb", that is.  ;)
The failures keep occurring, so you are right :) i'm sorry for the confusion. I will backout the other bug, can you reland this one or should we?
Flags: needinfo?(apavel) → needinfo?(bzbarsky)
Ted, do you have any idea how I can get a useful stack here?  minidump_stackwalk gives me the same thing as the logs, with no function info.  Can I trigger a symbol upload for a particular push so I can point minidump_stackwalk to those symbols?
Flags: needinfo?(ted)
Backed this out for now:

  https://hg.mozilla.org/mozilla-central/rev/9caf14e0b300
Status: RESOLVED → REOPENED
Flags: needinfo?(aryx.bugmail)
Resolution: FIXED → ---
We _can_ in fact call WrapObject() on a window. Stack:

 0  libxul.so!nsGlobalWindowInner::WrapObject [nsGlobalWindowInner.h:5baa664904e612e58c1234d13c9cb0f292876553 : 282 + 0x0]
 1  libxul.so!XPCConvert::NativeInterface2JSObject [XPCConvert.cpp:5baa664904e612e58c1234d13c9cb0f292876553 : 757 + 0x5]
 2  libxul.so!NativeInterface2JSObject [nsXPConnect.cpp:5baa664904e612e58c1234d13c9cb0f292876553 : 603 + 0xd]
 3  libxul.so!nsXPConnect::WrapNativeToJSVal [nsXPConnect.cpp:5baa664904e612e58c1234d13c9cb0f292876553 : 651 + 0xd]
 4  libxul.so!nsContentUtils::WrapNative [nsContentUtils.cpp:5baa664904e612e58c1234d13c9cb0f292876553 : 6774 + 0xd]
 5  libxul.so!xpcJSWeakReference::Get [nsContentUtils.h:5baa664904e612e58c1234d13c9cb0f292876553 : 2148 + 0xd]
 6  libxul.so!NS_InvokeByIndex [xptcinvoke_arm.cpp:5baa664904e612e58c1234d13c9cb0f292876553 : 167 + 0x3]

This involves us having a Window that is torn down enough that we have nulled out the wrapper, then someone trying to touch it from script.

For future reference, I got there by compiling google-breakpad (to get minidump_stackwalk) then grabbing symbols from the target.crashreporter-symbols.zip URL in the build log.
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Flags: needinfo?(ted)
Flags: needinfo?(bzbarsky)
Resolution: --- → INVALID
Depends on: 1442892
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.