1057135 - ServiceWorkerManager::Register() should use entry settings object

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Bug 1057135 - ServiceWorkerManager::Register() uses entry global

Comment 2

[(no longer active)]

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.

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cad3b206c267

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fab98234790

Comment 5

[Daniel Holbert [:dholbert]]

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

Comment 6

[Daniel Holbert [:dholbert]]

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.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Use MOZ_ASSERT

Simplifies the whole shebang.

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36271619da17

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/36271619da17

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

: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.

Comment 12

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 :)

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

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!