Comment on attachment 8477074 [details] [diff] [review] Bug 1057135 - ServiceWorkerManager::Register() uses entry global Review of attachment 8477074 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerManager.cpp @@ +555,5 @@ > MOZ_ASSERT(!nsContentUtils::IsCallerChrome()); > > + nsCOMPtr<nsIGlobalObject> sgo = GetEntryGlobal(); > + if (!sgo) { > + NS_WARNING("Register() should only be called from a valid entry settings object!"); Please use MOZ_ASSERT instead, we don't want people to get this wrong by accident. @@ +622,5 @@ > + return NS_ERROR_FAILURE; > + } > + > + nsCOMPtr<nsPIDOMWindow> window = do_QueryObject(sgo); > + if (!window) { I wonder if this can really happen... I guess better safe than sorry.

Per IRC discussion just now, the "return NS_ERROR_FAILURE;" line after the MOZ_CRASH (from comment 4) is unreachable and should be removed.

Looks like the unreachable "return NS_ERROR_FAILURE" from comment 5 still exists on inbound tip -- adding back needinfo=nsm to remove it in a followup.

https://hg.mozilla.org/mozilla-central/rev/cad3b206c267 https://hg.mozilla.org/mozilla-central/rev/0fab98234790

Simplifies the whole shebang.

:nsm, was this patch backed out, I see stuff landed on the 22nd and the 25th, but no real context in the bug about what it is? I ask because on the 22nd there was a 6% linux64 dromaeo dom regression which disappeared on the 25th.

nope it's still there, and I can't think of any way this patch would cause the regression, so I never acted on the dromaeo warning email I received :)

ok, it does appear that this change did cause a regression and I am only making an assumption it was fixed by the followup change. Either way we are not looking at a regression on firefox 34, so there is no need to worry!