ServiceWorkerManager::Register() should use entry settings object

RESOLVED FIXED in mozilla34

Status

()

Core
DOM: Workers
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: nsm, Assigned: nsm)

Tracking

unspecified
mozilla34
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
Created attachment 8477074 [details] [diff] [review]
Bug 1057135 - ServiceWorkerManager::Register() uses entry global
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Attachment #8477074 - Flags: review?(ehsan)

Comment 2

4 years ago
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.
Attachment #8477074 - Flags: review?(ehsan) → review+
Per IRC discussion just now, the "return NS_ERROR_FAILURE;" line after the MOZ_CRASH (from comment 4) is unreachable and should be removed.
Flags: needinfo?(nsm.nikhil)
Flags: needinfo?(nsm.nikhil)
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.
Flags: needinfo?(nsm.nikhil)
https://hg.mozilla.org/mozilla-central/rev/cad3b206c267
https://hg.mozilla.org/mozilla-central/rev/0fab98234790
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Created attachment 8478652 [details] [diff] [review]
Use MOZ_ASSERT

Simplifies the whole shebang.
Attachment #8478652 - Flags: review?(ehsan)

Updated

4 years ago
Attachment #8478652 - Flags: review?(ehsan) → review+
Flags: needinfo?(nsm.nikhil)
: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.
Flags: needinfo?(nsm.nikhil)
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 :)
Flags: needinfo?(nsm.nikhil)
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!

Updated

3 years ago
Depends on: 1173240
You need to log in before you can comment on or make changes to this bug.