1326023 - return a strong reference to the URLValue from Element::GetBindingURL

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

With the bug 1323671 patches, we can end up resolving a style context that is destroyed by the end of Element::GetBindingURL, thereby destroying the URLValue that we return a weak reference to.  We should make this return a strong reference.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Bug 1326023 - Make Element::GetBindingURL return a strong reference.

Review commit: https://reviewboard.mozilla.org/r/101140/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101140/

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

I don't really like the API that returns a RefPtr via a pointer out param, but looking at the function and its callsite, I cannot figure out any better way, so probably it's fine.

Strictly speaking I'm not a DOM peer, so passed it to smaug.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8822142 [details]
Bug 1326023 - Make Element::GetBindingURL return a strong reference.

https://reviewboard.mozilla.org/r/101140/#review101646

::: dom/base/Element.h:390
(Diff revision 1)
>      if (aNotify) {
>        UpdateState(true);
>      }
>    }
>  
> -  bool GetBindingURL(nsIDocument *aDocument, css::URLValue **aResult);
> +  bool GetBindingURL(nsIDocument* aDocument, RefPtr<css::URLValue>* aResult);

This is weird. Why not just use the normal style where out params are always addrefed, and then callers use getter_AddRefs.
Element::GetBindingURL needs to of course AddRef aResult when it points to non-null.

No need to use any unusual style here.

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8822142 [details]
Bug 1326023 - Make Element::GetBindingURL return a strong reference.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101140/diff/1-2/

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8822142 [details]
Bug 1326023 - Make Element::GetBindingURL return a strong reference.

https://reviewboard.mozilla.org/r/101140/#review101798

Hopefully the addref/release don't show up badly in the performance profiles or microbenchmarks (for XUL).

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf998120b814
Make Element::GetBindingURL return a strong reference. r=smaug

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for hazard failure:

https://hg.mozilla.org/integration/autoland/rev/a3174cbb505663f5665c1fc79ef7c24e61a01ef5

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=bf998120b81440e4d3809d2b5332e89e0831f30b
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=8492465&repo=autoland

[task 2016-12-29T12:13:32.934364Z] Found 1 hazards and 92 unsafe references
[task 2016-12-29T12:13:32.938581Z] + check_hazards /home/worker/workspace/analysis
[task 2016-12-29T12:13:32.938823Z] + set +e
[task 2016-12-29T12:13:32.939175Z] ++ grep -c 'Function.*has unrooted.*live across GC call' /home/worker/workspace/analysis/rootingHazards.txt
[task 2016-12-29T12:13:32.940576Z] + NUM_HAZARDS=1
[task 2016-12-29T12:13:32.940880Z] ++ grep -c '^Function.*takes unsafe address of unrooted' /home/worker/workspace/analysis/refs.txt
[task 2016-12-29T12:13:32.942005Z] + NUM_UNSAFE=92
[task 2016-12-29T12:13:32.942258Z] ++ grep -c '^Function.* has unnecessary root' /home/worker/workspace/analysis/unnecessary.txt
[task 2016-12-29T12:13:32.944807Z] + NUM_UNNECESSARY=1170
[task 2016-12-29T12:13:32.944839Z] + set +x
[task 2016-12-29T12:13:32.944859Z] TinderboxPrint: rooting hazards<br/>1
[task 2016-12-29T12:13:32.944883Z] TinderboxPrint: unsafe references to unrooted GC pointers<br/>92
[task 2016-12-29T12:13:32.944907Z] TinderboxPrint: unnecessary roots<br/>1170
[task 2016-12-29T12:13:32.944927Z] TEST-UNEXPECTED-FAIL 1 hazards detected

Comment 8

[Cameron McCormack (:heycam)]

Shuffled things around a bit to avoid the hazard: https://treeherder.mozilla.org/#/jobs?repo=try&revision=172b88c68394ce738c3aa18d3b576c2247fb93f3

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04dff9adc703
Make Element::GetBindingURL return a strong reference. r=smaug

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/04dff9adc703