Closed
Bug 1048595
Opened 10 years ago
Closed 10 years ago
Calling navigator.serviceWorker.register and then opening console crashes the browser
Categories
(Core :: DOM: Workers, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: tysmith, Assigned: tysmith)
References
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
##### index.html #######
<html>
<body>
<script>
if (!navigator.serviceWorker.active) {
navigator.serviceWorker.register('service.js')
} else {
console.log("Already have active serviceWorker " + navigator.serviceWorker.active);
}
</script>
</body>
</html>
##### service.js ########
oninstall = function(e) {
dump("\n\n\nGot INSTALL event\n\n\n");
}
#########################
To reproduce you need only to navigate to index.html and then open the browser console. The oninstall event fires.
Here's where it fails
thread #1: tid = 0x253a58, 0x0000000103550ae8 XUL`mozilla::dom::ServiceWorkerBinding::get_state(cx=0x00000001139f4170, obj=Handle<JSObject *> at 0x00007fff5fbe1bb8, self=0x000000012a5e4780, args=JSJitGetterCallArgs at 0x00007fff5fbe1ba8) + 232 at ServiceWorkerBinding.cpp:95, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
frame #0: 0x0000000103550ae8 XUL`mozilla::dom::ServiceWorkerBinding::get_state(cx=0x00000001139f4170, obj=Handle<JSObject *> at 0x00007fff5fbe1bb8, self=0x000000012a5e4780, args=JSJitGetterCallArgs at 0x00007fff5fbe1ba8) + 232 at ServiceWorkerBinding.cpp:95
92 MOZ_ASSERT(!JS_IsExceptionPending(cx));
93 {
94 // Scope for resultStr
-> 95 MOZ_ASSERT(uint32_t(result) < ArrayLength(ServiceWorkerStateValues::strings));
96 JSString* resultStr = JS_NewStringCopyN(cx, ServiceWorkerStateValues::strings[uint32_t(result)].value, ServiceWorkerStateValues::strings[uint32_t(result)].length);
97 if (!resultStr) {
98 return false;
full trace included in stacktrace file
Assignee | ||
Comment 2•10 years ago
|
||
Sure!
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → tylsmith
Comment 3•10 years ago
|
||
Let me know if you need any help. I'm not familiar with ServiceWorkers in particular, but I am a little familiar with DOM bindings code.
Assignee | ||
Comment 4•10 years ago
|
||
Thanks Andrew :)
Assignee | ||
Comment 5•10 years ago
|
||
Very simple. A variable wasn't being initialized.
Assignee | ||
Updated•10 years ago
|
Attachment #8468788 -
Flags: review?(bkelly)
Comment 6•10 years ago
|
||
Comment on attachment 8468788 [details] [diff] [review]
consolecrash.patch
Review of attachment 8468788 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/workers/ServiceWorker.cpp
@@ +16,5 @@
>
> ServiceWorker::ServiceWorker(nsPIDOMWindow* aWindow,
> SharedWorker* aSharedWorker)
> : DOMEventTargetHelper(aWindow),
> + mState(ServiceWorkerState::Installing),
Ah, yes. This definitely need to be initialized, but I'm not sure we should use Installing since its not really in that state yet.
So lets do this:
1) Add "unknown" to ServiceWorkerState in ServiceWorker.webidl to represent this constructed, but invalid state. Add a chrome-only comment next to it to indicate content should never see it.
2) Add a comment in ServiceWorker::GetState() to assert that Unknown is not returned unless we're in chrome code once bug 1043701 is fixed. (I think the debugger counts as chrome.)
We can't add the assert in (2) yet because the state code isn't implemented at all and the assert would always fire.
Please flag for re-review after making that change. Thanks!
Attachment #8468788 -
Flags: review?(bkelly) → review-
Assignee | ||
Comment 7•10 years ago
|
||
Is this okay?
Attachment #8468788 -
Attachment is obsolete: true
Attachment #8469405 -
Flags: review?(bkelly)
Comment 8•10 years ago
|
||
Comment on attachment 8468788 [details] [diff] [review]
consolecrash.patch
On second thought, lets just go with the simple solution for now. States are not really implemented yet. Handling the "constructed, but not installing yet" issue can be handled in bug 1043701.
Attachment #8468788 -
Attachment is obsolete: false
Attachment #8468788 -
Flags: review- → review+
Updated•10 years ago
|
Attachment #8469405 -
Attachment is obsolete: true
Attachment #8469405 -
Flags: review?(bkelly)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(continuation)
Comment 9•10 years ago
|
||
Comment on attachment 8469405 [details] [diff] [review]
consolecrash.patch
Review of attachment 8469405 [details] [diff] [review]:
-----------------------------------------------------------------
I would like to keep the WebIDL in sync with the spec.
Maybe we should know what the default state value is, and if this is not in the spec, we should file a bug.
How does it sound?
::: dom/webidl/ServiceWorker.webidl
@@ +29,5 @@
> "installed",
> "activating",
> "activated",
> + "redundant",
> + "unknown" // chrome only
I would prefer to have an internal state instead changing the WebIDL file.
::: dom/workers/ServiceWorker.cpp
@@ +16,5 @@
>
> ServiceWorker::ServiceWorker(nsPIDOMWindow* aWindow,
> SharedWorker* aSharedWorker)
> : DOMEventTargetHelper(aWindow),
> + mState(ServiceWorkerState::Unknown), // chrome-only
What about if mState is int32_t and we do -1 as default. Then we set MOZ_ASSERT in GetState?
Updated•10 years ago
|
Flags: needinfo?(bkelly)
Comment 10•10 years ago
|
||
Andrea, we actually chose not to go with that patch for now. I think the main issue here is mState is never populated or updated by any code because we need bug 1043701.
I came to the conclusion until we actually implement states, its probably not worth the effort to try to do any "unknown" state. Lets just use a valid state and then fix mState to use an intermediate state if necessary in bug 1043701.
Does that sound ok?
Flags: needinfo?(bkelly) → needinfo?(amarchesini)
Comment 11•10 years ago
|
||
I was going to land this, but it sounds like it is waiting on feedback from Andrea so I'll clear the flag for now.
Flags: needinfo?(continuation)
Comment 13•10 years ago
|
||
I spoke to Andrea over IRC and he is ok with landing this. I'll push it to mozilla-inbound.
Flags: needinfo?(amarchesini)
Comment 14•10 years ago
|
||
Status: NEW → ASSIGNED
Comment 15•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•