Open Bug 1408488 Opened 7 years ago Updated 5 days ago

Assertion failure: !mPendingReadyPromises.Contains(window), at dom/workers/ServiceWorkerManager.cpp:1414

Categories

(Core :: DOM: Service Workers, defect, P3)

58 Branch
x86_64
Linux
defect

Tracking

()

Tracking Status
firefox58 --- wontfix
firefox59 --- ?

People

(Reporter: MatsPalmgren_bugz, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: assertion)

Attachments

(1 file)

STR
1. create a new profile, then disable "Block pop-up windows" in Preferences
2. restart the browser, then load
http://lcamtuf.coredump.cx/cross_fuzz/cross_fuzz_randomized_20110105_seed.html

NOTE: this test may open a lot of windows, so I suggest you run it in a VM,
or if you're on Linux run it with a separate X server like Xephyr.
Blocks: crossfuzz
Component: DOM: Workers → DOM: Service Workers
My guess is the mReadyPromise is being cycle collected just before we try to access the promise again somehow.  Or somehow we are getting another ServiceWorkerContainer created somehow.

The safest fix here is to probably change this assert:

https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#1415

Into code that returns the existing promise if there is an entry for the window.

Probably can't easily write a test for this one.

Eden, do you want to look at this one as well after your current assertion fix bug?
Flags: needinfo?(echuang)
Priority: -- → P2
Yes, I will work on this bug after fix the current assertion bug.
Assignee: nobody → echuang
Status: NEW → ASSIGNED
Flags: needinfo?(echuang)
Instead of calling MOZ_ASSERT, the patch returns the existing pending ready promise directly.

bug 1411128 provides a trigger html to reproduce the bug, however, after applying this patch, the trigger html goes into an infinite loop.
Still to figure out how to let the case be a workable mochitest.
Attachment #8936999 - Flags: review?(bkelly)
Comment on attachment 8936999 [details] [diff] [review]
P1: Instead of calling assertion, return existing pending ready promise in serviceworker.ready. r?bkelly

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

r- for the MOZ_ASSERT() problem.

Note, its unclear to me what the test case should do if we don't hit the assert.  What do you mean by an "infinite loop"?  Maybe its supposed to do that?

We could also do something like check if the window is still an active window and throw NS_ERROR_DOM_INVALID_STATE_ERR if its not.  I think there is something like nsPIDOMWindowInner::IsActiveInnerWindow() or something like that.

::: dom/workers/ServiceWorkerManager.cpp
@@ +1320,5 @@
>    MOZ_ASSERT(!nsContentUtils::IsSystemPrincipal(doc->NodePrincipal()));
>  
> +  if (mPendingReadyPromises.Contains(window)) {
> +    PendingReadyPromise* data;
> +    MOZ_ASSERT(mPendingReadyPromises.Get(window, &data));

This will remove the mPendingReadyPromises.Get() call in non-DEBUG builds.  You probably want MOZ_ALWAYS_TRUE() instead.

But probably even better is using nsClassHashTable::LookupOrAdd() like:

  PendingReadyPromise* data = mPendingReadyPromises.LookupOrAdd(window);
  if (data->mPromise) {
    // return promise
  }
  // create promise.

This avoids performing two hash table lookups in this method.
Attachment #8936999 - Flags: review?(bkelly) → review-
Assignee: echuang → nobody
Status: ASSIGNED → NEW
Priority: P2 → P3
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: