Closed Bug 1057135 Opened 5 years ago Closed 5 years ago

ServiceWorkerManager::Register() should use entry settings object

Categories

(Core :: DOM: Workers, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: nsm, Assigned: nsm)

References

Details

Attachments

(1 file, 1 obsolete file)

Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Attached patch Use MOZ_ASSERTSplinter Review
Simplifies the whole shebang.
Attachment #8478652 - Flags: review?(ehsan)
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!
Depends on: 1173240
You need to log in before you can comment on or make changes to this bug.