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)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: tysmith, Assigned: tysmith)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached file stacktrace
##### 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
Nice find!  Tyler, can you track this down?
Sure!
Assignee: nobody → tylsmith
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.
Thanks Andrew :)
Very simple.  A variable wasn't being initialized.
Attachment #8468788 - Flags: review?(bkelly)
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-
Attached patch consolecrash.patch (obsolete) — Splinter Review
Is this okay?
Attachment #8468788 - Attachment is obsolete: true
Attachment #8469405 - Flags: review?(bkelly)
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+
Attachment #8469405 - Attachment is obsolete: true
Attachment #8469405 - Flags: review?(bkelly)
Flags: needinfo?(continuation)
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?
Flags: needinfo?(bkelly)
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)
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)
I spoke to Andrea over IRC and he is ok with landing this.  I'll push it to mozilla-inbound.
Flags: needinfo?(amarchesini)
https://hg.mozilla.org/mozilla-central/rev/87fd830d7c92
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.