Closed
Bug 1101819
Opened 10 years ago
Closed 9 years ago
unchecked call to ToJSValue in ServiceWorkerManager.cpp
Categories
(Core :: DOM: Workers, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: tromey, Assigned: tromey)
References
Details
Attachments
(1 file)
1.38 KB,
patch
|
nsm
:
review+
|
Details | Diff | Splinter Review |
While adding MOZ_WARN_UNUSED_RESULT to ToJSValue (see bug 1095728) I found some unchecked calls in ServiceWorkerManager.cpp. According to https://bugzilla.mozilla.org/show_bug.cgi?id=1095728#c2 this is "major nono". I have a patch, but basically cargo-culted from a bit later in RejectAllPromises.
Assignee | ||
Comment 1•10 years ago
|
||
Comment on attachment 8525582 [details] [diff] [review] check result of ToJSValue Review of attachment 8525582 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch Tom. ServiceWorkerManager is actively developed on the maple project branch, where UpdatePromise no longer exists. So this patch is not applicable. Thanks for bringing the correct use of ToJSValue to my attention though.
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Nikhil Marathe [:nsm] (away Nov 26-Nov 30) from comment #2) > ServiceWorkerManager is actively developed on the maple project branch, > where UpdatePromise no longer exists. So this patch is not applicable. > Thanks for bringing the correct use of ToJSValue to my attention though. Ok, fantastic. I will close this then. However I think the original patch (bug 1095728) still has to wait until this work is merged from the maple branch.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
If you mean that this patch should be landed since ServiceWorkerManager exists in the tree, ServiceWorkerManager is disabled by default, so I don't think it is a problem.
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Nikhil Marathe [:nsm] (away Nov 26-Nov 30) from comment #4) > If you mean that this patch should be landed since ServiceWorkerManager > exists in the tree, ServiceWorkerManager is disabled by default, so I don't > think it is a problem. The patch in question turns unchecked calls to ToJSValue into compile-time errors. So, it can't be merged as long as this code is part of the compilation.
Comment on attachment 8525582 [details] [diff] [review] check result of ToJSValue Review of attachment 8525582 [details] [diff] [review]: ----------------------------------------------------------------- Go ahead and land it :)
Attachment #8525582 -
Flags: review+
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Nikhil Marathe [:nsm] (away Nov 26-Nov 30) from comment #6) > Go ahead and land it :) Thanks very much!
Comment 8•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2ddaa3cbb7d
Keywords: checkin-needed
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e2ddaa3cbb7d
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•