Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2943693002:
Fix TryToCloseBrowserList not checking for null callbacks (Closed)
DescriptionFix TryToCloseBrowserList not checking for null callbacks
The bug cause crashes such as the one listed in the bug (733084). This
CL adds the proper null checks in TryToCloseBrowserList, with testing.
BUG=733804
Review-Url: https://codereview.chromium.org/2943693002
Cr-Commit-Position: refs/heads/master@{#480815}
Committed: https://chromium.googlesource.com/chromium/src/+/044a70558d6e54a54f2753382289eda3f120cee3
Patch Set 1 : Add offending tests #Patch Set 2 : Add the fix #Patch Set 3 : Incorporate treib@'s comment change into the CL. #
Messages
Total messages: 27 (17 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by lwchkg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lwchkg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lwchkg@chromium.org changed reviewers: + treib@chromium.org
Dear Marc, Please note the existence of this CL to avoid double work. (If you have already something concrete, feel free to pull my test to your CL.) Regards, WC Leung.
Non-owners LGTM, thanks! optional: You can copy the comment update in browser_list.h from https://chromium-review.googlesource.com/c/538773/
Description was changed from ========== Fix TryToCloseBrowserList not checking for null callbacks BUG=733084 ========== to ========== Fix TryToCloseBrowserList not checking for null callbacks The bug cause crashes such as the one listed in the bug (733084). This CL adds the proper null checks in TryToCloseBrowserList, with testing. BUG=733084 ==========
lwchkg@chromium.org changed reviewers: + jochen@chromium.org
Dear jochen@, PTAL as owner of the three files. Thanks! Regards, WC Leung.
The CQ bit was checked by lwchkg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
the referenced bug id appears to be unrelated? It's a bit strange that const refs are null. Can we change the call sites to always pass a non-null callback?
On 2017/06/19 13:08:09, jochen wrote: > the referenced bug id appears to be unrelated? The bug should be crbug.com/733804 > It's a bit strange that const refs are null. Can we change the call sites to > always pass a non-null callback? That's a somewhat weird property of our Callback classes: The default ctor creates them in a "null" state. (There was a recent discussion on chromium-dev about that - essentially, "historic reasons, not great but not worth the churn to change".) Of course we could say that this method requires non-null callbacks, but IMO that's a bit weird - it seems legitimate for a caller to say "I don't care about this callback" by passing in a default-constructed callback. Otherwise, they'd have to base::Bind an empty lambda or something along those lines.
Description was changed from ========== Fix TryToCloseBrowserList not checking for null callbacks The bug cause crashes such as the one listed in the bug (733084). This CL adds the proper null checks in TryToCloseBrowserList, with testing. BUG=733084 ========== to ========== Fix TryToCloseBrowserList not checking for null callbacks The bug cause crashes such as the one listed in the bug (733084). This CL adds the proper null checks in TryToCloseBrowserList, with testing. BUG=733804 ==========
Updated the issue number... sorry for the typo.
On 2017/06/19 13:15:43, Marc Treib wrote: > Of course we could say that this method requires non-null callbacks, but IMO > that's a bit weird - it seems legitimate for a caller to say "I don't care about > this callback" by passing in a default-constructed callback. Otherwise, they'd > have to base::Bind an empty lambda or something along those lines. I believe that it is commonplace to pass such default-constructed callback so we don't need an empty lambda, and empty callback functions affects performance because these functions cannot be put inline. BTW, I am surprised that Callback::Run() has undefined behavior for null callbacks, and it is valid to default-construct a null callback. This is a source of coding errors, and I don't know if there's any security concern. Unfortunately I've made the mistake of not null-checking callbacks for more than once. (If you think this is worth discussing, maybe we should put this on chromium-dev.)
lgtm
The CQ bit was checked by lwchkg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2943693002/#ps60001 (title: "Incorporate treib@'s comment change into the CL.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497964496594840,
"parent_rev": "7f5bf5e866ebc49436915bc515031cb377c6a939", "commit_rev":
"044a70558d6e54a54f2753382289eda3f120cee3"}
Message was sent while issue was closed.
Description was changed from ========== Fix TryToCloseBrowserList not checking for null callbacks The bug cause crashes such as the one listed in the bug (733084). This CL adds the proper null checks in TryToCloseBrowserList, with testing. BUG=733804 ========== to ========== Fix TryToCloseBrowserList not checking for null callbacks The bug cause crashes such as the one listed in the bug (733084). This CL adds the proper null checks in TryToCloseBrowserList, with testing. BUG=733804 Review-Url: https://codereview.chromium.org/2943693002 Cr-Commit-Position: refs/heads/master@{#480815} Committed: https://chromium.googlesource.com/chromium/src/+/044a70558d6e54a54f2753382289... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/044a70558d6e54a54f2753382289... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
