All users were logged out of Bugzilla on October 13th, 2018

Make dom::WarningOnlyErrorReporter handle workers

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks: 1 bug)

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(2 attachments, 1 obsolete attachment)

Now that we've done bug 1072144 we can do that.
(Assignee)

Comment 1

3 years ago
Created attachment 8725361 [details] [diff] [review]
Make dom::WarningOnlyErrorReporter handle workers
Attachment #8725361 - Flags: review?(bobbyholley)
(Assignee)

Updated

3 years ago
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8725361 [details] [diff] [review]
Make dom::WarningOnlyErrorReporter handle workers

Review of attachment 8725361 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/RuntimeService.cpp
@@ +802,5 @@
>      NS_WARNING("Could not create new context!");
>      return nullptr;
>    }
>  
> +  JS_SetErrorReporter(aRuntime, WarningOnlyErrorReporter);

Per IRC discussion, I think we should let ScriptSettings take care of this.
Attachment #8725361 - Flags: review?(bobbyholley) → review-
Whiteboard: btpp-active
(Assignee)

Comment 3

3 years ago
Created attachment 8725386 [details] [diff] [review]
Make dom::WarningOnlyErrorReporter handle workers
Attachment #8725386 - Flags: review?(bobbyholley)
(Assignee)

Updated

3 years ago
Attachment #8725361 - Attachment is obsolete: true
Comment on attachment 8725386 [details] [diff] [review]
Make dom::WarningOnlyErrorReporter handle workers

Review of attachment 8725386 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/base/CycleCollectedJSRuntime.cpp
@@ +502,5 @@
>                                          LargeAllocationFailureCallback, this);
>    JS_SetContextCallback(mJSRuntime, ContextCallback, this);
>    JS_SetDestroyZoneCallback(mJSRuntime, XPCStringConvert::FreeZoneCache);
>    JS_SetSweepZoneCallback(mJSRuntime, XPCStringConvert::ClearZoneCache);
> +  JS_SetErrorReporter(mJSRuntime, MozCrashErrorReporter);

Add a comment here saying that XPCJSRuntime currently override this because we don't TakeOwnership everwhere on the main thread yet.
Attachment #8725386 - Flags: review?(bobbyholley) → review+
Backed out along with the other things in the push for these failures in test_recusion.html: https://treeherder.mozilla.org/logviewer.html#?job_id=22804630&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/9716b46f3b60
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 8

3 years ago
Yep, the test_ctypes.xul fail is from this bug.  Will look into it.
(Assignee)

Comment 9

3 years ago
That failure happens because we have a test for using ctypes from a worker.  And ctypes uses js::PrepareScriptEnvironmentAndInvoke, which will report exceptions itself if there is no script environment preparer.  And right now we only set up an environment preparer for XPCJSRuntime.

I'm going to push that up to CycleCollectedJSRuntime, I think.
Flags: needinfo?(bzbarsky)
Created attachment 8725522 [details] [diff] [review]
part 1.  Push the script environment preparer bits up from XPCJSRuntime to CycleCollectedJSRuntime, because we need them on workers to do ctypes on workers properly

Note that I made some changes to the EnvironmentPreparer::invoke() impl to support running it on non-main threads.  So it's not just a cut/paste job there...
Attachment #8725522 - Flags: review?(bobbyholley)
Attachment #8725522 - Flags: review?(bobbyholley) → review+
Ugh, I suck.  I left in some debugging code...  Will reland with that taken out once the tree is reopened.
Flags: needinfo?(bzbarsky)

Comment 15

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e0a06595b43c
https://hg.mozilla.org/mozilla-central/rev/a74dae924b1f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.